Two failures on 2026-09-09, one cause each, both found by a 212-track album that would not convert. ── Symlinks ── paths.ts ran realpath FIRST and required the resolved path to be inside WORK_ROOT, so a link in the owner's home pointing at another disk was refused. On a machine whose home is BUILT from such links — ~/Music, ~/Movies, ~/Torrents each pointing at a mount — that leaves av-club able to see almost nothing. It refused every track of a folder the owner had open in the file browser at the time. That is the tell. The file browser decides what the owner may touch and it launches these jobs; its resolveUserPath checks the path as written and never calls realpath. Two confinement rules over the same files means the platform offers a right-click action its own sidecar cannot perform. What the strict rule was worth is smaller than it looks: this process runs AS the owner, so it reads nothing the owner cannot already read, and a link out of home reaches only more of the owner's disks. realpath-first was never the privilege boundary — the OS is. `..` is now refused rather than normalised, which is what realpath-first actually bought. Normalising is not what the kernel does: for /home/me/Music/.. the string says /home/me while the kernel follows Music to another filesystem first. Refusing removes the divergence instead of picking a side. The narrower claim that remains, and is the one to defend: a caller cannot NAME a path outside the root; they can only follow a link the owner put inside it. ── The token ── requireToken() threw when AV_CLUB_TOKEN was unset. That is right for a service a person starts with a shell and wrong for a sidecar the platform starts: not a safe default, a crash loop nobody sees. It was exactly that — 15 restarts, every proxied request 503, and an error that named the files rather than the missing variable. Generating one loses nothing: authorised() already accepts the platform's X-Officer-User header with no token at all, so no real caller carried this secret. It guards the direct loopback door, and 32 random bytes guard it better than a value copied into a shell profile. Kept 0600 beside the assets so replacing the working tree does not change the service's identity. AV_CLUB_TOKEN still wins when set. There is no unauthenticated mode; this only decides where the token comes from.
126 lines
7.1 KiB
TypeScript
126 lines
7.1 KiB
TypeScript
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.
|
|
*
|
|
* NO `..` SEGMENT, refused rather than normalised. This is what `realpath`-first used to buy, and it
|
|
* buys it without the cost below. Normalising `..` in the string is not the same operation the kernel
|
|
* performs: for `/home/me/Music/..`, `resolve` says `/home/me` while the kernel — following `Music`
|
|
* to another filesystem first — says the parent of the link's TARGET. Refusing the segment outright
|
|
* removes the divergence instead of picking a side, and no legitimate caller sends one.
|
|
*
|
|
* INSIDE THE ROOT AS WRITTEN, 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`.
|
|
*
|
|
* ── Symlinks LEAVE the root, deliberately. This reverses docs/SECURITY.md. ──
|
|
*
|
|
* The old rule ran `realpath` FIRST and required the resolved path to be inside the root, so a link in
|
|
* the owner's home pointing anywhere else was refused. On a machine whose home is BUILT from such
|
|
* links — `~/Music`, `~/Movies`, `~/Torrents` each pointing at a mounted disk — that rule means av-club
|
|
* can only see the few files physically on the home partition. On 2026-09-09 it refused all 212 tracks
|
|
* of an album with "inputPath is not a readable path", for a folder the owner was looking at in the
|
|
* file browser at the time.
|
|
*
|
|
* That is the tell. The file browser is the component that decides what the owner may touch, and it
|
|
* launched this job; its `resolveUserPath` checks the path AS WRITTEN and never calls `realpath`. Two
|
|
* different confinement rules over the same files means a right-click action the platform offers is one
|
|
* the sidecar structurally cannot perform. The file browser's rule is the correct one, because it is
|
|
* the one the owner can see the consequences of.
|
|
*
|
|
* What the strict rule was actually worth here is smaller than it looks. This process runs AS the owner,
|
|
* so it can read nothing the owner cannot already read, and a symlink out of home reaches only more of
|
|
* the owner's own disks. `realpath`-first was never an OS privilege boundary — the OS is that. It
|
|
* defends against a *path* being attacker-chosen, and absolute-only, no-`..`, and inside-the-root all
|
|
* still hold that line.
|
|
*
|
|
* The narrower claim that remains true: a caller cannot name a path outside the root. They can only
|
|
* follow a link the OWNER put inside it.
|
|
*/
|
|
|
|
/** 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);
|
|
|
|
/**
|
|
* A `..` anywhere in the path, as the caller wrote it.
|
|
*
|
|
* Checked on the RAW string, before any normalising, because normalising is the thing being avoided —
|
|
* see the header. Split on `sep` so a filename that merely contains dots (`..hidden`, `a..b.flac`) is
|
|
* untouched; only a whole segment counts.
|
|
*/
|
|
const hasDotDot = (raw: string) => raw.split(sep).includes("..");
|
|
|
|
/**
|
|
* 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<SafePath> {
|
|
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);
|
|
if (hasDotDot(raw)) throw invalid(`${field} must not contain '..'`, field);
|
|
|
|
const full = resolve(raw);
|
|
if (!inside(WORK_ROOT, full)) throw invalid(`${field} is not a readable path`, field);
|
|
|
|
// Resolved AFTER the check, and not re-checked: following the owner's own symlink off the home
|
|
// partition is the point. This still runs, because ffmpeg should be handed a path that cannot change
|
|
// meaning under it, and because a file that does not exist must fail here rather than inside a decoder.
|
|
const real = await realpath(full).catch(() => null);
|
|
if (!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<SafePath> {
|
|
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);
|
|
|
|
if (hasDotDot(raw)) throw invalid(`${field} must not contain '..'`, 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 parentAsWritten = full.slice(0, full.lastIndexOf(sep)) || sep;
|
|
if (!inside(WORK_ROOT, parentAsWritten)) throw invalid(`${field} is not a writable path`, field);
|
|
|
|
// As above: resolved after the check, so a write lands on the disk the owner's link points at rather
|
|
// than being refused. The parent must already exist — creating directories on the way to an output is
|
|
// a side effect nobody asked for, and a typo would scatter empty folders through somebody's home.
|
|
const parent = await realpath(parentAsWritten).catch(() => null);
|
|
if (!parent) throw invalid(`${field} is not a writable path`, field);
|
|
|
|
return `${parent}${sep}${name}` as SafePath;
|
|
}
|