Repository navigation
Conversation
…olicy and localize DASH/HLS inputs The media server fetched whatever URL a request named, and ffmpeg followed playlist entries and redirects on its own. Every remote read (inputs, HLS/DASH resources, thumbnails, probes, recording verification, audio, webhooks, parallel ranged downloads) now goes through fetchAllowed: only http(s), no loopback/private/link-local/metadata/CGNAT/ULA addresses unless the host is trusted or listed in MEDIA_SERVER_ALLOWED_HOSTS, and each redirect hop is checked again. HLS and DASH manifests are downloaded and rewritten to local files before ffmpeg runs with a file,pipe protocol whitelist. In DASH, MPD@Location no longer changes the media base and sibling BaseURLs resolve against their parent. The webhook secret is compared in constant time.
|
@07rjain is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await assertUrlAllowed(current, policy, resolve); | ||
| const response = await fetcher(current, withManualRedirect(init)); |
There was a problem hiding this comment.
fetchAllowed checks DNS, then gives the original hostname to fetcher, which resolves it again. A hostname in an attacker-controlled playlist can return a public address for the check and a private address for the connection. This leaves the private-address fix bypassable.
Bind each connection to an address that passed the check, including in resolveAllowedUrl.
How this was verified: Playlist resources reach fetchAllowed, but the addresses checked by assertUrlAllowed are never passed to the network connection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/url-policy.ts
Line: 285-286
Comment:
**DNS changes bypass the check**
`fetchAllowed` checks DNS, then gives the original hostname to `fetcher`, which resolves it again. A hostname in an attacker-controlled playlist can return a public address for the check and a private address for the connection. This leaves the private-address fix bypassable.
Bind each connection to an address that passed the check, including in `resolveAllowedUrl`.
**How this was verified:** Playlist resources reach `fetchAllowed`, but the addresses checked by `assertUrlAllowed` are never passed to the network connection.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const response = await fetchAllowed(webhookUrl, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| "x-media-server-secret": webhookSecret, | ||
| }, | ||
| body: JSON.stringify({ kind: "audio-levels", ...body }), | ||
| redirect: "error", | ||
| signal, |
There was a problem hiding this comment.
The audio-level callback previously refused redirects. It now follows them through fetchAllowed, which sends the same x-media-server-secret and JSON body to every target. With the default policy, a redirect to another public host passes, exposing the callback secret and recording details to that host.
Keep redirect refusal for this callback, and make fetchAllowed respect redirect: "error".
How this was verified: The callback includes the secret header, and the redirect loop reuses those headers and the body without checking whether the origin changed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/audio-levels.ts
Line: 78-85
Comment:
**Redirects receive the callback secret**
The audio-level callback previously refused redirects. It now follows them through `fetchAllowed`, which sends the same `x-media-server-secret` and JSON body to every target. With the default policy, a redirect to another public host passes, exposing the callback secret and recording details to that host.
Keep redirect refusal for this callback, and make `fetchAllowed` respect `redirect: "error"`.
**How this was verified:** The callback includes the secret header, and the redirect loop reuses those headers and the body without checking whether the origin changed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| "-protocol_whitelist", | ||
| "file,pipe", |
There was a problem hiding this comment.
getAudioExtractArgs now allows only file,pipe. AES-128 HLS still needs FFmpeg's crypto protocol after the key and segments have been downloaded locally, so encrypted HLS audio extraction now fails. Include crypto here, as getAudioProbeArgs and the video download command already do.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/media-audio.ts
Line: 123-124
Comment:
**Encrypted HLS audio fails**
`getAudioExtractArgs` now allows only `file,pipe`. AES-128 HLS still needs FFmpeg's `crypto` protocol after the key and segments have been downloaded locally, so encrypted HLS audio extraction now fails. Include `crypto` here, as `getAudioProbeArgs` and the video download command already do.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if (resolved.includes("$")) { | ||
| throw new Error("DASH template URL cannot be fetched locally"); |
There was a problem hiding this comment.
The generic DASH fallback now rejects every URL containing $, but the HLS converter does not cover all valid template shapes. For example, it does not read a SegmentTemplate inherited from Period. That manifest falls back here and throws on $Number$.
Previously the generic MPD kept the template for FFmpeg to expand. Expand these templates into checked local resources before rejecting the fallback.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/media-video.ts
Line: 458-459
Comment:
**DASH fallback rejects valid templates**
The generic DASH fallback now rejects every URL containing `$`, but the HLS converter does not cover all valid template shapes. For example, it does not read a `SegmentTemplate` inherited from `Period`. That manifest falls back here and throws on `$Number$`.
Previously the generic MPD kept the template for FFmpeg to expand. Expand these templates into checked local resources before rejecting the fallback.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| return attributes.replace( | ||
| /(initialization|media|sourceURL|xlink:href|href)="([^"]+)"/gi, |
There was a problem hiding this comment.
rewriteDashAttributes only matches double-quoted attributes with no spaces around =. Valid XML such as <SegmentURL media='seg-1.m4s'/> is left unchanged, so its resource is never downloaded.
This worked before because FFmpeg could fetch the remote resource. With directory BaseURL elements removed and network protocols disabled, FFmpeg now looks for a nonexistent local file or rejects the remote URL. Parse these attributes with the same quote and whitespace support as parseXmlAttributes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/media-video.ts
Line: 471-472
Comment:
**Valid XML references stay remote**
`rewriteDashAttributes` only matches double-quoted attributes with no spaces around `=`. Valid XML such as `<SegmentURL media='seg-1.m4s'/>` is left unchanged, so its resource is never downloaded.
This worked before because FFmpeg could fetch the remote resource. With directory `BaseURL` elements removed and network protocols disabled, FFmpeg now looks for a nonexistent local file or rejects the remote URL. Parse these attributes with the same quote and whitespace support as `parseXmlAttributes`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| return await downloadAllowedResource( | ||
| resolved, | ||
| dirPath, | ||
| abortSignal, | ||
| limit, | ||
| ); |
There was a problem hiding this comment.
Each HLS resource occurrence now downloads the entire URL into a fresh file. A byte-range playlist can name one large media file hundreds of times, so this downloads and stores hundreds of full copies instead of the requested ranges. Cache downloads by resolved URL, or download the named ranges and rewrite their offsets.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/media-video.ts
Line: 394-399
Comment:
**HLS downloads duplicate whole files**
Each HLS resource occurrence now downloads the entire URL into a fresh file. A byte-range playlist can name one large media file hundreds of times, so this downloads and stores hundreds of full copies instead of the requested ranges. Cache downloads by resolved URL, or download the named ranges and rewrite their offsets.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| test("probe does not fetch the redirect target", async () => { | ||
| process.env.MEDIA_SERVER_ALLOW_PRIVATE_URLS = "false"; | ||
| const target = privateRedirectServer(); | ||
| try { | ||
| await expect(probeVideo(target.url)).rejects.toThrow("private address"); | ||
| expect(target.secretHits()).toBe(0); |
There was a problem hiding this comment.
These tests start with a private loopback URL while private URLs are disabled. The policy rejects the first URL, so the redirect response is never reached. All five tests would still pass if the corresponding download path followed unchecked redirects after allowing the initial URL.
Make the first hop allowed, redirect it to a different blocked destination, and assert that the first hop was requested.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/__tests__/lib/redirect-follow.test.ts
Line: 51-56
Comment:
**Redirect tests miss the redirect**
These tests start with a private loopback URL while private URLs are disabled. The policy rejects the first URL, so the redirect response is never reached. All five tests would still pass if the corresponding download path followed unchecked redirects after allowing the initial URL.
Make the first hop allowed, redirect it to a different blocked destination, and assert that the first hop was requested.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Thanks @07rjain for splitting this out of #2580. Having the URL policy on its own makes it much easier to review, and the
CI hasn't run on this fork yet, and a maintainer needs to approve it. Once the above is in and the branch is rebased, we'll need Greptile at 5/5 and green CI before we can merge. After pushing, you can mention @greptileai in a comment to get a re-review. Thanks again! |
| const directory = await mkdtemp(join(tmpdir(), "cap-recording-")); | ||
| const path = join(directory, "recording.mp4"); | ||
| const cleanup = () => rm(directory, { force: true, recursive: true }); |
There was a problem hiding this comment.
For storage URLs that materializeMedia cannot handle, stageRemoteRecording saves the whole recording under tmpdir()/cap-recording-*. This is outside the managed temp root, so disk reports miss these files and cleanup after a crash never removes them. Large or abandoned recordings can fill the disk unnoticed.
Stage through the managed temp helpers so these files are counted and swept.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/recording-verification.ts
Line: 742-744
Comment:
**Recording files escape cleanup**
For storage URLs that `materializeMedia` cannot handle, `stageRemoteRecording` saves the whole recording under `tmpdir()/cap-recording-*`. This is outside the managed temp root, so disk reports miss these files and cleanup after a crash never removes them. Large or abandoned recordings can fill the disk unnoticed.
Stage through the managed temp helpers so these files are counted and swept.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| await Promise.all( | ||
| [...required].map(async (url) => { | ||
| localByUrl.set( | ||
| url, | ||
| await downloadAllowedResource(url, dirPath, abortSignal, limit), | ||
| ); | ||
| return `${attribute}="${escapeXmlAttribute(resolved)}"`; | ||
| }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
If one required DASH segment returns 404, Promise.all rejects while sibling downloads keep running and the limiter starts more queued downloads. downloadStreamingVideoToTemp then removes the directory and returns the error without stopping or waiting for those transfers. Repeated failures can leave downloads running after their requests end and their slots become available.
Abort sibling downloads and wait for them to finish before cleanup. The HLS and converted DASH download groups need the same treatment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/media-video.ts
Line: 600-607
Comment:
**Failed jobs keep downloading**
If one required DASH segment returns 404, `Promise.all` rejects while sibling downloads keep running and the limiter starts more queued downloads. `downloadStreamingVideoToTemp` then removes the directory and returns the error without stopping or waiting for those transfers. Repeated failures can leave downloads running after their requests end and their slots become available.
Abort sibling downloads and wait for them to finish before cleanup. The HLS and converted DASH download groups need the same treatment.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if (primary) urls.push(resolved); | ||
| else alternatives.push({ url: resolved, element }); |
There was a problem hiding this comment.
For a SegmentBase manifest with a first BaseURL that returns 404 and a working sibling URL, this code makes the first URL required. Its download fails before any alternative is tried, so the video cannot be processed even though a working copy exists.
Treat sibling BaseURL entries as choices and fail only when no allowed, working choice remains.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/media-video.ts
Line: 546-547
Comment:
**DASH backups are never tried**
For a `SegmentBase` manifest with a first `BaseURL` that returns 404 and a working sibling URL, this code makes the first URL required. Its download fails before any alternative is tried, so the video cannot be processed even though a working copy exists.
Treat sibling `BaseURL` entries as choices and fail only when no allowed, working choice remains.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| localInput = /^https?:/i.test(inputPath) | ||
| ? await downloadVideoToTemp(inputPath, undefined, abortSignal) | ||
| : { path: inputPath, cleanup: async () => {} }; |
There was a problem hiding this comment.
The new remote download runs outside the thumbnail's 60-second deadline. downloadVideoToTemp allows a normal download to take ten minutes, after which the frame decoder receives another full 60 seconds. A slow source can therefore keep /video/thumbnail waiting far longer than before.
Apply the thumbnail deadline to the download too, then give the decoder only the remaining time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/media-video.ts
Line: 2262-2264
Comment:
**Thumbnail downloads miss their deadline**
The new remote download runs outside the thumbnail's 60-second deadline. `downloadVideoToTemp` allows a normal download to take ten minutes, after which the frame decoder receives another full 60 seconds. A slow source can therefore keep `/video/thumbnail` waiting far longer than before.
Apply the thumbnail deadline to the download too, then give the decoder only the remaining time.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| async start() { | ||
| try { | ||
| const staged = await stageRemoteInput( | ||
| videoUrl, | ||
| abortController.signal, | ||
| ); |
There was a problem hiding this comment.
In /audio/convert, extractAudioStream now downloads inside asynchronous start(), but the route immediately returns a successful audio response. If an allowed public URL redirects to a private address, start() rejects after that response has been returned. The caller gets a failed audio stream instead of the promised 400 URL_NOT_ALLOWED JSON response.
Wait for staging and the URL checks before returning the response.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/media-audio.ts
Line: 543-548
Comment:
**Blocked audio returns broken streams**
In `/audio/convert`, `extractAudioStream` now downloads inside asynchronous `start()`, but the route immediately returns a successful audio response. If an allowed public URL redirects to a private address, `start()` rejects after that response has been returned. The caller gets a failed audio stream instead of the promised 400 `URL_NOT_ALLOWED` JSON response.
Wait for staging and the URL checks before returning the response.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| return await decodeRecording( | ||
| staged.path, | ||
| options, | ||
| undefined, | ||
| sourceAudioInput, | ||
| packetProof, |
There was a problem hiding this comment.
Direct calls to verifyRecording with a remote URL reset their timeout after downloading. The recursive call receives the original options, starts a new timer, and never receives the outer deadline's signal. A download that uses most of timeoutMs therefore gets another full decode budget and can return success after the requested deadline.
Pass the combined signal and remaining time into the local decode so callers can rely on their requested timeout.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/recording-verification.ts
Line: 842-847
Comment:
**Recording checks reset their timeout**
Direct calls to `verifyRecording` with a remote URL reset their timeout after downloading. The recursive call receives the original `options`, starts a new timer, and never receives the outer deadline's signal. A download that uses most of `timeoutMs` therefore gets another full decode budget and can return success after the requested deadline.
Pass the combined signal and remaining time into the local decode so callers can rely on their requested timeout.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Preserve upstream segment pooling, uploads during verification and recording diagnostics. Check cancellation by client request starts and aborted signals so late server arrivals do not make the regression test flaky.
Apply the IPv4-compatible, NAT64 and site-local blocks already used by merged PR CapSoftware#1932. Cover literal endpoint rejection and preserve public IPv4-mapped IPv6 support.
|
Thanks @richiemcilroy for the detailed review. The fixes are pushed through d4aee16, and the branch now includes current main (3614bdb); GitHub reports no merge conflicts. The updates cover managed recording staging and transfer budgets, connection-time IP pinning, redirect refusal for secret-bearing callbacks, encrypted HLS audio, inherited DASH templates and XML attribute handling, stable HLS resource caching, and redirect tests that exercise the first hop. They also cover cancellation and cleanup, DASH backup URLs, and shared staging/decode deadlines. I compared the policy with merged #1932 and aligned its IPv4-compatible IPv6, NAT64 and site-local restrictions, with regression tests. Local validation: 287 targeted media tests passed with one existing skip after merging main; the follow-up IPv6 change passed 41 media tests and 69 web endpoint-policy tests. Root typecheck, scoped Biome checks and media-server bundling passed. Direct media-server TypeScript checking still reports the same seven diagnostics reproduced on unchanged main; a combined media test run also has existing shared-mock interference, so I am not claiming that full run is green. The extra probe HEAD request is removed. Remote thumbnail generation still stages the whole input, now within its shared 60-second deadline; the ranged-read optimization remains outstanding. All seven GitHub workflows on the latest commit are awaiting maintainer approval with zero jobs started, and both Vercel previews require team authorization. Could you approve the fork workflows and take another look when you have time? Thanks! |
Split out of #2580 (media-server URL policy and DASH handling).
Remote media inputs, redirects, playlist resources and upload/webhook URLs now pass through one HTTP(S) URL policy. Loopback, private, link-local, CGNAT and IPv6 ULA destinations are refused unless explicitly trusted. Configured storage/web origins remain supported, including bundled MinIO.
Upgrade note: private storage must be configured through S3_INTERNAL_ENDPOINT or MEDIA_SERVER_ALLOWED_HOSTS on the media server. Bundled Compose deployments already forward their trusted endpoints. The new materialization limits and proxy behavior are documented in the self-hosting guide.
Validation: scoped Biome checks and media-server bundling pass. URL-policy/redirect, DASH/HLS materialization, request-budget, audio/video integration, recording verification, route, transfer, job, audio-level and resource-cleanup suites pass when run in isolation. Recording verification passed 127 tests with its existing long-recording skip under FFmpeg 7; audio-level integration passed all 16 under FFmpeg 8. Media-server TypeScript checking reports the same seven diagnostics on unchanged main. A combined Bun test run encounters shared mocks leaking between existing suites; it is not reported as green.
Upstream compatibility: merged main at
3614bdb69, integrating 174 upstream commits. The current segment-download pool, multipart uploads during verification, and recording diagnostics are retained. On the merged branch, 287 targeted tests passed with one existing long-recording skip;bun run typecheck, scoped Biome checks and media-server bundling passed. Media-specific TypeScript diagnostics remain identical to current upstream main.The PR needs fixes before merging because private-network protection remains bypassable and several media flows still fail.
Findings
Fix with agent prompt
Summary
The PR adds a shared URL policy and downloads remote playlist resources before FFmpeg reads them.
Review was based on code inspection. No runtime tests were run.
Reviews (2) · Last reviewed commit: "fix(media-server): check every remote UR..." · Reviewed by Greptile