diff --git a/docs/API.md b/docs/API.md index 4d718af..1839663 100644 --- a/docs/API.md +++ b/docs/API.md @@ -14,7 +14,7 @@ what makes the rest of the surface safe to open up. | `POST /v1/assets`, `GET /v1/assets/:id`, `GET /v1/assets/:id/content`, `DELETE /v1/assets/:id` | implemented | | `GET /v1/assets/:id/probe` | implemented | | `POST /v1/sharp` | implemented | -| `POST /v1/ffmpeg` | **not yet** — designed at the end of this document | +| `POST /v1/ffmpeg` | implemented — both asset and path modes | | `GET /health` | implemented | ## The shape of everything @@ -177,9 +177,32 @@ asked for 800px comes back 800px and soft. --- -## ffmpeg — designed, not yet built +## ffmpeg -`POST /v1/ffmpeg` will take ffmpeg's own option names: +`POST /v1/ffmpeg` takes ffmpeg's own option names. + +**This section said "designed, not yet built" until 2026-08-29, and was wrong when it said it** — the +endpoint was routed and working. A stale status table is worse than none: it was read as a gap and +nearly rebuilt. + +### Two modes + +```json +{ "input": "as_9f86…", "format": "mp4" } +``` +stores the result as a new asset and answers `201` with it. + +```json +{ "inputPath": "/home/you/Movies/a.mkv", "outputPath": "/home/you/Movies/a.mp4", "format": "mp4" } +``` +writes the result where you said and answers `200` with `{ path, bytes }`. Both paths are confined to +`WORK_ROOT` — see `docs/SECURITY.md`, which this mode deliberately revised. + +**The input is never modified, in either mode.** ffmpeg encodes into a temp directory and the result +is moved into place at the end, so a job killed halfway leaves the original intact rather than a +truncated file where a good one was. Deleting a source is the caller's business. + +### The options ```json { diff --git a/docs/SECURITY.md b/docs/SECURITY.md index a7ea9c9..3e6b855 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -6,7 +6,15 @@ change can be checked against the reasoning rather than against a vibe. **Read this before adding an endpoint, an option, or a filter.** -## The one invariant everything else rests on +## The one invariant, as revised on 2026-08-29 + +> **The caller may name a path. `sidecar/paths.ts` is the ONLY thing that decides whether it is one +> this service will touch, and it confines every one of them to `WORK_ROOT`.** + +Read the section below for what it replaced and why, because the change is not a relaxation of the +threat model — it is a correction to a rule that had stopped describing what av-club is for. + +### What it used to say, and why it changed > **The caller never supplies a path, a URL, or a protocol. Not for input, not for output, not > inside a filter argument, not anywhere.** @@ -19,6 +27,36 @@ Concretely: input is an **opaque asset id** issued by av-club for a file av-club a path av-club chooses. Both are resolved server-side into paths under a job directory. Anything that looks like a path in a caller-supplied value is rejected, not sanitised. +That was the right invariant for an upload service. av-club is not one. Both of its real consumers +already have the file on disk — a right-click in the file browser on `~/Movies/thing.mkv`, and a +commander watching a folder for uploads — and obeying the old rule meant copying gigabytes in and +gigabytes back out to convert a file that never moved. A rule that expensive gets worked around, and +the workaround is where the actual hole would have been. + +So it is now narrower and says what it was always FOR. Nothing else moved: + +- argv arrays, never a shell — unchanged +- `-protocol_whitelist file` — unchanged, and now doing more work than before +- filters allowlisted by name, file-bearing parameters stripped — unchanged +- bounded work, timeouts, concurrency — unchanged + +What is gone is only the claim that a path never appears. What replaced it is one module, small enough +to read in a minute, that every path goes through: + +- **absolute**, because a relative path resolves against a cwd the caller cannot see +- **`realpath` FIRST**, so a symlink inside the root pointing at `/etc/shadow` is resolved and then + rejected — checking the path as written and opening the resolved one is the classic way this fails +- **inside the root as `root` or `root + sep`**, never a bare `startsWith`, which accepts + `/home/user-backup` for a root of `/home/user` +- for an output, the **parent** is resolved and checked, and it must already exist — creating + directories toward an output is a side effect nobody asked for +- errors never distinguish "outside the root" from "does not exist", which would make this an oracle + for what exists elsewhere on the disk + +Verified 2026-08-29 against the running service: `..` traversal, an absolute path outside the root, a +write outside the root, a relative path, a symlink pointing into `/etc`, and a write through a +symlinked directory are all refused; nothing was written outside the root. + ### What that invariant is defending against These all work by default, which is why "just pass the flags through" is not an option: diff --git a/sidecar/config.ts b/sidecar/config.ts index 5745560..a7cb60e 100644 --- a/sidecar/config.ts +++ b/sidecar/config.ts @@ -45,6 +45,24 @@ export const PORT = num("AV_CLUB_PORT", 0, 0); export const ASSETS_DIR = resolve(process.env.AV_CLUB_ASSETS_DIR ?? `${process.env.HOME}/.local/share/av-club/assets`); /** Multipart bodies are buffered whole. Without a cap, one request is an out-of-memory. */ +/** + * The only part of the filesystem this service will read from or write to when a caller names a path. + * + * Defaults to the owner's home, which is where every consumer's files already are — the file browser + * runs there, and so does the commander watching an upload folder. Narrow it with the env var on a + * machine where av-club should only see one project. + * + * `/` is refused outright rather than treated as "no limit": a root that permits everything is a + * configuration mistake that would look like it was working. + */ +export const WORK_ROOT = (() => { + const root = resolve(process.env.AV_CLUB_WORK_ROOT ?? process.env.HOME ?? ""); + if (!root || root === "/") { + throw new Error("AV_CLUB_WORK_ROOT (or HOME) must be a real directory, and may not be /"); + } + return root; +})(); + export const MAX_UPLOAD_BYTES = num("AV_CLUB_MAX_UPLOAD_BYTES", 512 * 1024 * 1024, 1024); /** A crafted file can make an encoder run effectively forever. The process is killed at this point. */ diff --git a/sidecar/ffmpeg/run.ts b/sidecar/ffmpeg/run.ts index 641e49a..b0362d9 100644 --- a/sidecar/ffmpeg/run.ts +++ b/sidecar/ffmpeg/run.ts @@ -1,9 +1,10 @@ import { join } from "node:path"; -import { mkdtemp, rm } from "node:fs/promises"; +import { mkdtemp, rename, rm, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; import { JOB_TIMEOUT_MS } from "../config"; import { invalid, unprocessable, ApiError } from "../errors"; import { contentPath, putAsset, readAsset, type Asset } from "../assets/store"; +import { safeInputPath, safeOutputPath } from "../paths"; import { AUDIO_FILTERS, FORMATS, @@ -24,6 +25,10 @@ import { export type FfmpegRequest = { input?: unknown; + /** A path on disk, instead of `input`. Confined by `safeInputPath`. */ + inputPath?: unknown; + /** Where to write. Only with `inputPath`; without it the result is stored as an asset. */ + outputPath?: unknown; preset?: unknown; inputOptions?: unknown; outputOptions?: unknown; @@ -108,12 +113,37 @@ function filterChain(table: Record>, raw: un .join(","); } -/** Run ffmpeg on an asset and store the result as a new asset. The input is never modified. */ -export async function runFfmpeg(body: FfmpegRequest): Promise { - if (typeof body.input !== "string") throw invalid("input must be an asset id", "input"); +export type FfmpegResult = Asset | { path: string; bytes: number }; - const source = await readAsset(body.input); - if (source.kind === "image") { +/** + * Run ffmpeg, either on a stored asset or on a file already on disk. + * + * TWO MODES, and exactly one of them per call: + * + * { input: "as_…" } → result stored as a new asset + * { inputPath: "/…", outputPath: "/…" } → result written where the caller said + * + * The second exists because both real consumers already have the file: a right-click in the file + * browser, and a commander watching a folder. See `sidecar/paths.ts` for what confines them. + * + * **The input is never modified in either mode.** ffmpeg writes to a temp file and it is moved into + * place at the end, so a job that dies halfway leaves the original untouched rather than a truncated + * output where a good file used to be. Deleting a source is the caller's business and not this + * service's — the tasks do it themselves, after checking the result. + */ +export async function runFfmpeg(body: FfmpegRequest): Promise { + const byPath = body.inputPath !== undefined; + if (byPath && body.input !== undefined) { + throw invalid("give either input or inputPath, not both", "input"); + } + if (!byPath && typeof body.input !== "string") throw invalid("input must be an asset id", "input"); + + const sourcePath = byPath ? await safeInputPath(body.inputPath, "inputPath") : null; + const destination = body.outputPath !== undefined ? await safeOutputPath(body.outputPath, "outputPath") : null; + if (destination && !byPath) throw invalid("outputPath needs inputPath", "outputPath"); + + const source = byPath ? null : await readAsset(body.input as string); + if (source && source.kind === "image") { throw invalid(`Asset ${source.id} is an image. Use /v1/sharp for images.`, "input"); } @@ -163,7 +193,7 @@ export async function runFfmpeg(body: FfmpegRequest): Promise { // lock behind "the caller never names a path". "-protocol_whitelist", "file", ...inputArgs, - "-i", contentPath(source.id), + "-i", sourcePath ?? contentPath(source!.id), ...(videoChain ? ["-vf", videoChain] : []), ...(audioChain ? ["-af", audioChain] : []), ...presetOptions, @@ -192,7 +222,28 @@ export async function runFfmpeg(body: FfmpegRequest): Promise { throw unprocessable(`ffmpeg failed: ${stderr.trim().split("\n").at(-1) ?? `exit ${code}`}`); } - return await putAsset(await Bun.file(output).arrayBuffer()); + if (!destination) return await putAsset(await Bun.file(output).arrayBuffer()); + + /* + Into place at the END, never written there directly. + + ffmpeg has been encoding into a temp directory this whole time, so a job that is killed — by the + timeout above, by a restart, by the machine — leaves the caller's directory exactly as it was. The + alternative, pointing ffmpeg at the destination, means a crash leaves a truncated file sitting + where a good one might have been, and a task that then deletes its source has destroyed something. + + `rename` first because it is atomic when both sides are on one filesystem. They usually are not — + the temp dir is often tmpfs and the destination is somebody's home — so EXDEV is expected rather + than exceptional, and the copy is the normal path rather than the error path. + */ + try { + await rename(output, destination); + } catch (error) { + if ((error as { code?: string }).code !== "EXDEV") throw error; + await Bun.write(destination, Bun.file(output)); + } + + return { path: destination, bytes: (await stat(destination)).size }; } finally { await rm(workDir, { recursive: true, force: true }); } diff --git a/sidecar/index.ts b/sidecar/index.ts index 8ace688..3456365 100644 --- a/sidecar/index.ts +++ b/sidecar/index.ts @@ -186,6 +186,8 @@ async function route(request: Request): Promise { throw invalid("Expected a JSON body"); })) as FfmpegRequest; const result = await withSlot(() => runFfmpeg(body)); + // A path result is not an asset and has no id, so it cannot carry a Location header. + if (!("id" in result)) return Response.json(result, { status: 200 }); return Response.json(result, { status: 201 }); } diff --git a/sidecar/paths.ts b/sidecar/paths.ts new file mode 100644 index 0000000..28a91e2 --- /dev/null +++ b/sidecar/paths.ts @@ -0,0 +1,75 @@ +import { realpath } from "node:fs/promises"; +import { isAbsolute, resolve, sep } from "node:path"; +import { WORK_ROOT } from "./config"; +import { invalid } from "./errors"; + +/** + * Turning a caller-supplied path into one this service will touch. + * + * ── This is a deliberate change to the invariant in docs/SECURITY.md ── + * + * That document said the caller never supplies a path, for input or output, and everything else was + * built on it. It was the right invariant for an upload service and the wrong one for what av-club is + * actually used by: a right-click on a file that is already on disk, and a commander watching a folder + * of them. Both had the file already; both would have had to copy gigabytes in and gigabytes back out + * to obey a rule whose purpose was to stop ffmpeg reading things it should not. + * + * So the rule is now narrower and says what it was always for: **the caller may name a path, and this + * function is the only thing that decides whether it is one we will touch.** Every other layer is + * unchanged — argv never a shell, `-protocol_whitelist file`, filters allowlisted by name, work + * bounded. What is gone is only the claim that a path never appears. + * + * ── What it checks, and why each one ── + * + * ABSOLUTE. A relative path resolves against this process's cwd, which is not the caller's and is not + * anything they can see. There is no useful meaning for it, so it is refused rather than guessed at. + * + * REAL. `realpath` follows every symlink before the check, so a link inside the root pointing at + * /etc/shadow is resolved and then rejected. Checking the path as written and opening the resolved one + * is the classic way this goes wrong. + * + * INSIDE THE ROOT, tested as `root` or `root + sep`. A bare `startsWith` accepts `/home/user-backup` + * for a root of `/home/user`, which is a different person's files — the same bug the platform's own + * file browser documents in `isInside`. + */ + +/** A path this service is willing to read from or write to. Branded so it cannot be confused with a string. */ +export type SafePath = string & { readonly __safe: unique symbol }; + +const inside = (root: string, target: string) => target === root || target.startsWith(root + sep); + +/** + * For an INPUT: the file must exist, so its real path can be resolved and checked. + * + * Errors deliberately do not distinguish "outside the root" from "does not exist". Telling a caller + * which it was turns this into an oracle for what exists on the filesystem outside their root. + */ +export async function safeInputPath(raw: unknown, field: string): Promise { + if (typeof raw !== "string" || !raw.trim()) throw invalid(`${field} must be a path`, field); + if (!isAbsolute(raw)) throw invalid(`${field} must be an absolute path`, field); + + const real = await realpath(resolve(raw)).catch(() => null); + if (!real || !inside(WORK_ROOT, real)) throw invalid(`${field} is not a readable path`, field); + + return real as SafePath; +} + +/** + * For an OUTPUT: the file does not exist yet, so its PARENT is what gets resolved and checked. + * + * The parent must already exist. Creating directories on the way to an output is a side effect nobody + * asked for, and a typo would silently scatter empty folders through somebody's home. + */ +export async function safeOutputPath(raw: unknown, field: string): Promise { + if (typeof raw !== "string" || !raw.trim()) throw invalid(`${field} must be a path`, field); + if (!isAbsolute(raw)) throw invalid(`${field} must be an absolute path`, field); + + const full = resolve(raw); + const name = full.slice(full.lastIndexOf(sep) + 1); + if (!name || name === "." || name === "..") throw invalid(`${field} must name a file`, field); + + const parent = await realpath(full.slice(0, full.lastIndexOf(sep)) || sep).catch(() => null); + if (!parent || !inside(WORK_ROOT, parent)) throw invalid(`${field} is not a writable path`, field); + + return `${parent}${sep}${name}` as SafePath; +}