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! |
|
@greptileai please re-review the latest changes on commit d4aee16. The displayed 0/5 review predates the fixes for managed recording staging, connection-time IP pinning, callback redirect refusal, encrypted HLS, DASH inheritance/XML/BaseURL handling, resource caching and cancellation, redirect coverage, and shared deadlines. I am also rechecking every item from Richie's review against this head. |
| const inputPath = join(dirPath, "input"); | ||
| await Bun.write(inputPath, response); |
There was a problem hiding this comment.
stageRemoteInput now downloads every ordinary remote audio/video input in full with Bun.write, without a byte limit. /audio/convert allows this download to run for 15 minutes. A large or continuous response can fill temporary storage before the timeout runs, causing other recordings and uploads to fail. The output-size check does not limit this input download.
Use a bounded streaming download and count its bytes against any enclosing transfer budget.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/media-audio.ts
Line: 205-206
Comment:
**Audio downloads can fill disk**
`stageRemoteInput` now downloads every ordinary remote audio/video input in full with `Bun.write`, without a byte limit. `/audio/convert` allows this download to run for 15 minutes. A large or continuous response can fill temporary storage before the timeout runs, causing other recordings and uploads to fail. The output-size check does not limit this input download.
Use a bounded streaming download and count its bytes against any enclosing transfer budget.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const candidates = ["GET", "HEAD"].includes( | ||
| (init.method ?? "GET").toUpperCase(), | ||
| ) | ||
| ? addresses | ||
| : addresses.slice(0, 1); |
There was a problem hiding this comment.
If a server has no working IPv6 connection and a public storage hostname returns IPv6 first, fetchChecked tries only that address for PUT and POST. It never tries the working IPv4 address. Downloads can succeed because GET and HEAD try all checked addresses, but uploads fail. Upload retries repeat the same first-address choice.
Preserve address fallback for connection failures before sending the request, without replaying a body that may already have been sent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/url-policy.ts
Line: 253-257
Comment:
**Uploads skip working addresses**
If a server has no working IPv6 connection and a public storage hostname returns IPv6 first, `fetchChecked` tries only that address for PUT and POST. It never tries the working IPv4 address. Downloads can succeed because GET and HEAD try all checked addresses, but uploads fail. Upload retries repeat the same first-address choice.
Preserve address fallback for connection failures before sending the request, without replaying a body that may already have been sent.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
@greptileai please re-review commit c8166f9. Both new P1 findings are fixed:
Validation: 332 tests passed across 11 isolated suites, with one existing long-recording skip; scoped Biome, bundling and diff checks pass. Media-server TypeScript still reports seven existing errors in unchanged files. I also rechecked Richie's earlier correctness findings and simulated a merge with main at d2744bc: no conflicts. Remote thumbnail generation still stages the full input; its shared 60-second deadline is enforced, and the suggested ranged-read optimization remains open. |
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| const family = isIP(address); | ||
| if (family === 4) return blockedRanges.check(address, "ipv4"); | ||
| if (family === 6) { | ||
| const mapped = address.toLowerCase().match(/^::ffff:(\d+\.\d+\.\d+\.\d+)$/); |
There was a problem hiding this comment.
P1: IPv4-mapped IPv6 literals bypass the new private-address URL policy
URL canonicalization turns [::ffff:169.254.169.254] into ::ffff:a9fe:a9fe, which the new dotted-only mapped-address check and IPv6 block list both miss.
Block ::ffff:0:0/96 for IPv6 (or canonicalize mapped addresses) and test mapped literals through assertUrlAllowed.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="apps/media-server/src/lib/url-policy.ts">
<violation number="1" location="apps/media-server/src/lib/url-policy.ts:37">
<priority>P1</priority>
<title>IPv4-mapped IPv6 literals bypass the new private-address URL policy</title>
<evidence>The new policy only recognizes IPv4-mapped IPv6 addresses in dotted form: `address.toLowerCase().match(/^::ffff:(\d+\.\d+\.\d+\.\d+)$/)`. WHATWG URL parsing canonicalizes mapped literals to hex before this check runs, so `http://[::ffff:169.254.169.254]/...` reaches `isPrivateAddress` as `::ffff:a9fe:a9fe`. The dotted regex then fails and `blockedRanges.check(address, "ipv6")` does not block `::ffff:0:0/96` because no such subnet was added. The value is therefore classified as public. `checkedAddresses` uses the literal directly (`isIP(host) ? [host] : await resolve(host)`) and `fetchChecked` pins the connection to that address while preserving the original Host/TLS name, so the media server connects to the private destination. The added test only exercises the dotted string directly and `::`/NAT64 forms, not the canonicalized mapped form that real URLs produce.</evidence>
<recommendation>Add the IPv4-mapped IPv6 range `::ffff:0:0/96` to `blockedRanges` for `ipv6`, or canonicalize/parse mapped addresses with `node:net` before classification so both dotted and hex forms are checked against the IPv4 block list. Add regression tests that call `assertUrlAllowed` (not only `isPrivateAddress`) with `http://[::ffff:127.0.0.1]/`, `http://[::ffff:169.254.169.254]/`, and an equivalent redirect/Location target.</recommendation>
</violation>
</file>
|
@greptileai please re-review the latest head, 13b33fa. This includes the earlier P1 fixes and main merged at d92c21a. The Docker failures are addressed: the images load their test configuration, and the newly merged cancellation/DASH tests now verify localized resources correctly. Production-image checks also caught an incompatibility with main's Loom importer; manifest staging now leaves segment retries, selection, byte limits and missing-segment tolerance to its existing prefetch pass. Cached verification-run requests added on main now use the checked URL policy, with a regression showing a blocked redirect falls back to normal verification successfully. Validation: 647 production-image tests passed across 17 groups, including 70 Loom importer tests. Two existing performance cases were skipped; arm64 and the dedicated performance checks remain for CI. Scoped Biome, bundling and whitespace checks pass; the seven existing media-specific TypeScript diagnostics remain. The additional scanner report about canonicalized IPv4-mapped IPv6 addresses was tested in the production runtime: these private destinations are already blocked by BlockList. New direct-URL and redirect coverage confirms both dotted and hexadecimal forms are refused. |
|
Hi @richiemcilroy, could you take another look when you have a chance? The review fixes are pushed through 13b33fa, with main merged at d92c21a. Greptile is now 5/5, all seven GitHub workflows passed (including both amd64 and arm64 Docker builds), and the latest Superagent security scan passed. The older mapped-IPv6 security comment remains unresolved, but the claimed bypass was not reproducible in the production image: BlockList already rejects those private mapped addresses. Added direct-URL and redirect coverage confirms this. The PR description has the validation details. The thumbnail ranged-read optimization remains outstanding; download and frame extraction share the enforced 60-second deadline. Thanks again for the detailed review! |
|
Hi @richiemcilroy, could you take another look at this PR when you have a chance? Greptile is 5/5, all seven GitHub workflows and the latest security scan passed on 13b33fa, and I reran 137 focused tests against the current checkout in the production container—all passed. The older mapped-IPv6 warning remains visible as an unresolved thread, but the private mapped addresses are blocked in the production runtime and covered by direct-URL and redirect tests. The two remaining red checks are Vercel deployment authorization gates. Thanks! |
Split out of #2580 (media-server URL policy and DASH handling).
Remote media inputs, redirects, playlist resources, uploads and callbacks now pass through a shared HTTP(S) URL policy. Private, loopback, link-local, CGNAT and IPv6 local destinations are blocked unless explicitly trusted. Configured storage/web endpoints and bundled MinIO remain supported.
Upgrade note: configure private storage through S3_INTERNAL_ENDPOINT or MEDIA_SERVER_ALLOWED_HOSTS on the media server. Bundled Compose deployments already forward trusted endpoints. Materialization limits and proxy behavior are documented in the self-hosting guide.
Upstream compatibility: merged main at
d92c21a61on October 26, retaining its recording verification, packet proofs, cancellation, preverification and Loom importer changes. The branch has no commits missing from that main snapshot.Validation: built the amd64 production image with Bun 1.4.4 and its packaged FFmpeg. Across the workflow's normal test groups and the additional URL-policy, redirect, download, request-budget, audio and preverification groups, 647 tests passed across 17 groups. Two existing performance tests were skipped; the dedicated performance checks and arm64 validation remain for CI. Scoped Biome checks, diff whitespace checks and media-server bundling pass. Media-server TypeScript checking still reports seven existing diagnostics in unchanged code; it is not reported as green. Tests were run in separate processes matching the workflow's grouping because unrelated shared mocks leak in a combined workspace run.
Security scan follow-up: the reported canonicalized IPv4-mapped IPv6 bypass is not reproducible under Bun 1.4.4 in the production image: BlockList already applies the private IPv4 subnets to mapped IPv6 addresses. Added direct-URL and redirect tests for both dotted and hexadecimal mapped loopback/metadata forms, while retaining support for public mapped addresses.
Remaining optimization: remote thumbnails still stage the full input before decoding one frame. Their shared 60-second deadline is enforced.
The PR appears safe to merge; all previous findings are addressed and no new blocking issue was established.Summary
The PR checks remote media connections and redirect destinations before fetching them.
Reviews (5) · Last reviewed commit: "fix(media-server): secure upstream verif..." · Reviewed by Greptile