plugins/av-club
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

a symlink the owner put inside the root may lead out of it

Browse Source 
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.
This commit is contained in:
pastilhas
2026-09-09 23:34:39 +01:00
parent 62972c1146
commit a467000b3d
3 changed files with 138 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
sidecar/config.ts
+51 -10
View File
@@ -1,4 +1,6 @@
import { resolve } from "node:path";
import { randomBytes } from "node:crypto";
import { mkdirSync, readFileSync, writeFileSync } from "node:fs";
import { dirname, resolve } from "node:path";
/**
* Configuration, read once at boot.
@@ -127,16 +129,55 @@ export const MAX_CONCURRENT_JOBS = num("AV_CLUB_MAX_CONCURRENT_JOBS", 2, 1);
* read — the doc generator imports the sharp vocabulary, which imports MAX_OUTPUT_DIMENSION from
* here — and a module-scope throw meant that generating documentation required a production secret.
*
* The check itself still happens at boot: server.ts calls this before it listens, so a missing token
* is a refusal to start rather than a service that quietly accepts everyone. There is no
* "unauthenticated by default" mode; this process runs decoders over untrusted bytes.
* `AV_CLUB_TOKEN` still wins when it is set — that is how the standalone deployment pins a value
* something else already knows. When it is not set, one is generated and kept (see below) rather than
* refused. There is still no "unauthenticated by default" mode: the door always wants a token, this
* only decides where that token comes from.
*/
export function requireToken(): string {
const value = process.env.AV_CLUB_TOKEN;
if (!value || value.length < 16) {
throw new Error(
"AV_CLUB_TOKEN must be set and at least 16 characters. Generate one with: openssl rand -hex 32",
);
const fromEnv = process.env.AV_CLUB_TOKEN;
if (fromEnv) {
if (fromEnv.length < 16) {
throw new Error("AV_CLUB_TOKEN, when set, must be at least 16 characters.");
}
return fromEnv;
}
return value;
return tokenFromStateDir();
}
/**
* The token, generated and kept on disk when nothing supplied one.
*
* This used to throw, and that was wrong for a plugin. A sidecar is started by the platform, not by a
* person with a shell — so "refuse to boot until an operator exports a variable" is not a safe default,
* it is a crash loop nobody sees. On 2026-09-09 it was exactly that: av-club restarted 15 times, and
* every request the platform proxied to it came back 503. A whole 212-file conversion failed with an
* error that named the files, not the missing variable.
*
* Generating one loses nothing. `authorised()` already accepts the platform's `X-Officer-User` header
* without a token at all, so no real caller was ever carrying this secret — it only guards the direct
* loopback door, and a random 32 bytes guards that better than a value copied into a shell profile.
*
* Written 0600 next to the assets, for the same reason they live there: a deploy that replaces the
* working tree must not change the service's identity.
*/
function tokenFromStateDir(): string {
const file = resolve(ASSETS_DIR, "..", "token");
// Missing is the normal first-boot case, not an error. Anything else — a directory in the way, a
// permission problem — is also survivable: fall through and mint a fresh one rather than refuse to start.
const existing = (() => {
try {
return readFileSync(file, "utf8").trim();
} catch {
return "";
}
})();
if (existing.length >= 16) return existing;
const generated = randomBytes(32).toString("hex");
mkdirSync(dirname(file), { recursive: true });
writeFileSync(file, `${generated}\n`, { mode: 0o600 });
return generated;
}
sidecar/paths.ts
+60 -10
View File
@@ -24,13 +24,39 @@ import { invalid } from "./errors";
* 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.
* 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, 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`.
* 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. */
@@ -38,6 +64,15 @@ 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.
*
@@ -47,9 +82,16 @@ const inside = (root: string, target: string) => target === root || target.start
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 real = await realpath(resolve(raw)).catch(() => null);
if (!real || !inside(WORK_ROOT, real)) throw invalid(`${field} is not a readable path`, 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;
}
@@ -64,12 +106,20 @@ export async function safeOutputPath(raw: unknown, field: string): Promise<SafeP
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 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);
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;
}