Fix file access, XSS and access-control bugs from a security scan

- Media rows can no longer be created through the CRUD API, updates only
  take description fields on the caller's own row, and every media read
  or delete goes through mediaFilePath, which refuses paths outside
  DATA_DIR or inside the database folder.
- Uploads strip the file name and extension to safe characters and keep
  only title, description and summary from the request's media object.
- Folder imports no longer follow symlinks (walkRegularFiles), and the
  source path is quoted in the computer's shell.
- User and bot files are served with nosniff, and HTML, SVG, XML and
  scripts are sandboxed by CSP.
- Sandbox cookies are kept to the computer's prefix, and websocket
  upgrades on /ws and the computer proxy must come from the app's own
  address (README and Help note nginx's Host header).
- user_types CRUD writes go through the assignment matrix; members only
  read their own user record; read-only users can read CRUD again;
  grabUsers builds valid SQL for array search terms and bad pages; an
  admin media DELETE needs an id; read-only users can't delete media.
- Rate limits and the audit log use the connection's address, trusting
  forwarded headers only from a proxy, and login limits are split per
  address, per account from that address and per account overall.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

src/utils/client-ip.test.ts

@@ -0,0 +1,38 @@
+import { describe, expect, test } from "bun:test";
+import clientIP, { isProxyAddress } from "./client-ip";
+
+describe("isProxyAddress", () => {
+    test("loopback and private networks", () => {
+        for (const ip of ["127.0.0.1", "::1", "::ffff:127.0.0.1", "10.8.0.1", "192.168.1.4", "172.18.0.2", "fd00::1"]) {
+            expect(isProxyAddress({ ip })).toBe(true);
+        }
+    });
+
+    test("public addresses", () => {
+        for (const ip of ["8.8.8.8", "172.32.0.1", "2001:db8::1"]) {
+            expect(isProxyAddress({ ip })).toBe(false);
+        }
+    });
+});
+
+describe("clientIP", () => {
+    test("a direct client is its connection's address, whatever it claims", () => {
+        expect(clientIP({ peer: "203.0.113.9", forwarded_for: "1.2.3.4" })).toBe("203.0.113.9");
+    });
+
+    test("behind a proxy, the entry that proxy added", () => {
+        expect(clientIP({ peer: "127.0.0.1", forwarded_for: "1.2.3.4, 203.0.113.9" })).toBe("203.0.113.9");
+    });
+
+    test("behind a proxy that sends X-Real-IP only", () => {
+        expect(clientIP({ peer: "172.18.0.1", real_ip: "203.0.113.9" })).toBe("203.0.113.9");
+    });
+
+    test("a LAN client with no proxy headers is its own address", () => {
+        expect(clientIP({ peer: "192.168.1.20" })).toBe("192.168.1.20");
+    });
+
+    test("nothing known", () => {
+        expect(clientIP({})).toBeUndefined();
+    });
+});

src/utils/client-ip.ts

@@ -0,0 +1,59 @@
+type Params = {
+    /**
+     * The connection's own address (`server.requestIP`).
+     */
+    peer?: string | null;
+    /**
+     * The request's `X-Forwarded-For`.
+     */
+    forwarded_for?: string | null;
+    /**
+     * The request's `X-Real-IP`.
+     */
+    real_ip?: string | null;
+};
+
+/**
+ * Whether an address is this machine or a private network: where a reverse
+ * proxy or tunnel in front of the app connects from.
+ */
+export function isProxyAddress({ ip }: { ip: string }) {
+    const address = ip.replace(/^::ffff:/i, "").toLowerCase();
+
+    return (
+        address == "::1" ||
+        address.startsWith("127.") ||
+        address.startsWith("10.") ||
+        address.startsWith("192.168.") ||
+        /^172\.(1[6-9]|2\d|3[01])\./.test(address) ||
+        address.startsWith("fc") ||
+        address.startsWith("fd") ||
+        address.startsWith("fe80:")
+    );
+}
+
+/**
+ * The client's address, for rate limits and the audit log. The forwarded
+ * headers are only believed when the connection came from a proxy
+ * (`isProxyAddress`), and then only the last `X-Forwarded-For` entry,
+ * the one that proxy added itself: anything before it is whatever the
+ * client chose to send. Otherwise the connection's own address wins, so a
+ * client can't pick a fresh address per request to dodge a limit.
+ */
+export default function clientIP({ peer, forwarded_for, real_ip }: Params) {
+    const behind_proxy = !peer || isProxyAddress({ ip: peer });
+
+    if (behind_proxy) {
+        const forwarded = (forwarded_for || "")
+            .split(",")
+            .map((entry) => entry.trim())
+            .filter(Boolean)
+            .at(-1);
+
+        if (forwarded) return forwarded;
+
+        if (real_ip?.trim()) return real_ip.trim();
+    }
+
+    return peer || undefined;
+}

src/utils/computer-proxy-response-headers.test.ts

@@ -0,0 +1,44 @@
+import { describe, expect, test } from "bun:test";
+import { AppData } from "@/src/data/app-data";
+import computerProxyResponseHeaders, { computerProxyCookies } from "./computer-proxy-response-headers";
+
+const prefix = "/computers/3";
+
+describe("computerProxyCookies", () => {
+    test("keeps a sandbox cookie to the computer's prefix", () => {
+        expect(
+            computerProxyCookies({ cookies: ["key=abc; Path=/; HttpOnly; Domain=example.com"], prefix }),
+        ).toEqual(["key=abc; HttpOnly; Path=/computers/3/"]);
+    });
+
+    test("drops a cookie named like the app's session", () => {
+        expect(
+            computerProxyCookies({
+                cookies: [`${AppData.AuthKeyCookieName}=forged; Path=/`, `${AppData.AuthCSRFCookieName}=x`, "ok=1"],
+                prefix,
+            }),
+        ).toEqual(["ok=1; Path=/computers/3/"]);
+    });
+});
+
+describe("computerProxyResponseHeaders", () => {
+    test("rewrites every Set-Cookie and keeps framing open", () => {
+        const headers = new Headers();
+
+        headers.append("set-cookie", "a=1; Path=/");
+        headers.append("set-cookie", `${AppData.AuthKeyCookieName}=forged; Path=/`);
+        headers.set("x-frame-options", "DENY");
+
+        const out = computerProxyResponseHeaders({ headers, base_url: "http://computer:8080", prefix });
+
+        expect(out.getSetCookie()).toEqual(["a=1; Path=/computers/3/"]);
+        expect(out.get("x-frame-options")).toBeNull();
+    });
+
+    test("points a redirect back under the prefix", () => {
+        const headers = new Headers({ location: "http://computer:8080/vnc/index.html" });
+        const out = computerProxyResponseHeaders({ headers, base_url: "http://computer:8080", prefix });
+
+        expect(out.get("location")).toBe("/computers/3/vnc/index.html");
+    });
+});

src/utils/computer-proxy-response-headers.ts

@@ -1,3 +1,5 @@
+import { AppData } from "@/src/data/app-data";
+
 type Params = {
     headers: Headers;
     /**
@@ -25,6 +27,18 @@ export default function computerProxyResponseHeaders({ headers, base_url, prefix
 
     for (const name of Dropped) out.delete(name);
 
+    /**
+     * The sandbox is served on the app's own origin, so a cookie it sets
+     * would land beside the app's session: each one is kept to the
+     * computer's own prefix, and one named like the app's session cookies
+     * is dropped.
+     */
+    out.delete("set-cookie");
+
+    for (const cookie of computerProxyCookies({ cookies: headers.getSetCookie(), prefix })) {
+        out.append("set-cookie", cookie);
+    }
+
     const csp = out.get("content-security-policy");
 
     if (csp?.includes("frame-ancestors")) {
@@ -51,3 +65,23 @@ export default function computerProxyResponseHeaders({ headers, base_url, prefix
 
     return out;
 }
+
+/**
+ * A computer's `Set-Cookie` values, each limited to `Path=<prefix>` (any
+ * `Path` or `Domain` it named removed), without the app's own cookie
+ * names.
+ */
+export function computerProxyCookies({ cookies, prefix }: { cookies: string[]; prefix: string }) {
+    const reserved: string[] = [AppData.AuthKeyCookieName, AppData.AuthCSRFCookieName, AppData.BuildCookieName];
+
+    return cookies
+        .filter((cookie) => !reserved.includes(cookie.split("=")[0]!.trim()))
+        .map((cookie) => {
+            const [pair, ...attributes] = cookie.split(";");
+            const kept = attributes
+                .map((attribute) => attribute.trim())
+                .filter((attribute) => attribute && !/^(path|domain)\s*=/i.test(attribute));
+
+            return [pair!.trim(), ...kept, `Path=${prefix}/`].join("; ");
+        });
+}

src/utils/is-same-origin-upgrade.test.ts

@@ -0,0 +1,54 @@
+import { describe, expect, test } from "bun:test";
+import isSameOriginUpgrade from "./is-same-origin-upgrade";
+
+function upgrade({ headers }: { headers: Record<string, string> }) {
+    return new Request("http://localhost:7227/ws", { headers });
+}
+
+describe("isSameOriginUpgrade", () => {
+    test("the app's own page is let through", () => {
+        expect(
+            isSameOriginUpgrade({
+                req: upgrade({ headers: { origin: "http://10.8.0.1:7227", host: "10.8.0.1:7227" } }),
+            }),
+        ).toBe(true);
+    });
+
+    test("behind a proxy that forwards the host", () => {
+        expect(
+            isSameOriginUpgrade({
+                req: upgrade({
+                    headers: {
+                        origin: "https://cobble.example.com",
+                        host: "127.0.0.1:7227",
+                        "x-forwarded-host": "cobble.example.com",
+                    },
+                }),
+            }),
+        ).toBe(true);
+    });
+
+    test("another port on the same machine is refused", () => {
+        expect(
+            isSameOriginUpgrade({
+                req: upgrade({ headers: { origin: "http://localhost:8080", host: "localhost:7227" } }),
+            }),
+        ).toBe(false);
+    });
+
+    test("another site is refused", () => {
+        expect(
+            isSameOriginUpgrade({
+                req: upgrade({ headers: { origin: "https://evil.example", host: "cobble.example.com" } }),
+            }),
+        ).toBe(false);
+    });
+
+    test("a malformed or opaque origin is refused", () => {
+        expect(isSameOriginUpgrade({ req: upgrade({ headers: { origin: "null", host: "localhost:7227" } }) })).toBe(false);
+    });
+
+    test("a client without an origin is left to the sign-in check", () => {
+        expect(isSameOriginUpgrade({ req: upgrade({ headers: { host: "localhost:7227" } }) })).toBe(true);
+    });
+});

src/utils/is-same-origin-upgrade.ts

@@ -0,0 +1,38 @@
+type Params = {
+    req: Request;
+};
+
+/**
+ * Whether a websocket upgrade came from a page of the app itself. A
+ * websocket handshake isn't bound by CORS, and the session cookies are
+ * `SameSite=Lax`, so a page on the same site but another origin (another
+ * port on the same machine, a sandbox's exposed port) would otherwise
+ * reach `/ws` or a computer's terminal with the user's session.
+ *
+ * The browser's `Origin` must name the host the request was made to: the
+ * `Host` header, or the proxy's `X-Forwarded-Host` when a proxy rewrote
+ * it. A client that sends no `Origin` isn't a browser page and carries no
+ * ambient cookies of its own, so it is let through to the usual sign-in
+ * check.
+ */
+export default function isSameOriginUpgrade({ req }: Params) {
+    const origin = req.headers.get("origin");
+
+    if (!origin) return true;
+
+    let origin_host = "";
+
+    try {
+        origin_host = new URL(origin).host.toLowerCase();
+    } catch {
+        return false;
+    }
+
+    if (!origin_host) return false;
+
+    const hosts = [req.headers.get("host"), ...(req.headers.get("x-forwarded-host") || "").split(",")]
+        .map((host) => (host || "").trim().toLowerCase())
+        .filter(Boolean);
+
+    return hosts.includes(origin_host);
+}

src/utils/media-file-path.test.ts

@@ -0,0 +1,32 @@
+import { describe, expect, test } from "bun:test";
+import mediaFilePath from "./media-file-path";
+
+const data_dir = "/data";
+const db_dir = "/data/db";
+
+describe("mediaFilePath", () => {
+    test("resolves a path inside the data directory", () => {
+        expect(mediaFilePath({ write_path: "media/3/a.png", data_dir, db_dir })).toBe("/data/media/3/a.png");
+        expect(mediaFilePath({ write_path: "bots/2/report.md", data_dir, db_dir })).toBe("/data/bots/2/report.md");
+    });
+
+    test("refuses a path that climbs out of the data directory", () => {
+        expect(mediaFilePath({ write_path: "../app/.env", data_dir, db_dir })).toBeNull();
+        expect(mediaFilePath({ write_path: "media/../../etc/passwd", data_dir, db_dir })).toBeNull();
+    });
+
+    test("refuses an absolute path", () => {
+        expect(mediaFilePath({ write_path: "/app/.env", data_dir, db_dir })).toBeNull();
+    });
+
+    test("refuses the database folder", () => {
+        expect(mediaFilePath({ write_path: "db/cobble.db", data_dir, db_dir })).toBeNull();
+        expect(mediaFilePath({ write_path: "media/../db/cobble.db", data_dir, db_dir })).toBeNull();
+    });
+
+    test("refuses empty paths, the root itself and NUL bytes", () => {
+        expect(mediaFilePath({ write_path: "", data_dir, db_dir })).toBeNull();
+        expect(mediaFilePath({ write_path: ".", data_dir, db_dir })).toBeNull();
+        expect(mediaFilePath({ write_path: "media/a\0.png", data_dir, db_dir })).toBeNull();
+    });
+});

src/utils/media-file-path.ts

@@ -0,0 +1,41 @@
+import path from "node:path";
+import grabDirNames from "./grab-dir-names";
+import { isInsideRoot } from "./resolve-bot-path";
+
+type Params = {
+    /**
+     * A `media` row's `media_write_path` or `media_thumbnail_write_path`,
+     * relative to the data directory (e.g. `media/3/abc.png`).
+     */
+    write_path?: string | null;
+    /**
+     * The data directory, else `DATA_DIR` (tests pass their own).
+     */
+    data_dir?: string;
+    /**
+     * The database directory, else `DB_DIR` (tests pass their own).
+     */
+    db_dir?: string;
+};
+
+/**
+ * The absolute path of a media row's file, or null when the row points
+ * anywhere it must not: outside the data directory (`../app/.env`, an
+ * absolute path) or into the database's own folder. Every read or delete
+ * of a media file goes through this, so a row's path can never become a
+ * way onto the rest of the disk.
+ */
+export default function mediaFilePath({ write_path, data_dir, db_dir }: Params): string | null {
+    if (!write_path || write_path.includes("\0")) return null;
+
+    const root = path.resolve(data_dir || grabDirNames().DATA_DIR);
+    const absolute = path.resolve(root, write_path);
+
+    if (absolute == root || !isInsideRoot({ root, absolute })) return null;
+
+    const database = path.resolve(db_dir || process.env.DB_DIR || path.join(root, "db"));
+
+    if (isInsideRoot({ root: database, absolute })) return null;
+
+    return absolute;
+}

src/utils/request-peer-ip.ts

@@ -0,0 +1,14 @@
+/**
+ * The address each request's connection came from, as `Bun.serve` saw it
+ * (`server.requestIP`). Recorded by `src/server.ts` before the request is
+ * handed on, since API routes only get the `Request`.
+ */
+const peers = new WeakMap<Request, string>();
+
+export function setRequestPeerIP({ req, ip }: { req: Request; ip?: string | null }) {
+    if (ip) peers.set(req, ip);
+}
+
+export function grabRequestPeerIP({ req }: { req: Request }) {
+    return peers.get(req);
+}

src/utils/served-file-headers.test.ts

@@ -0,0 +1,40 @@
+import { describe, expect, test } from "bun:test";
+import servedFileHeaders, { isActiveContentType } from "./served-file-headers";
+
+describe("isActiveContentType", () => {
+    test("pages, SVG, XML and scripts are active", () => {
+        for (const content_type of [
+            "text/html",
+            "text/html; charset=utf-8",
+            "TEXT/HTML",
+            "image/svg+xml",
+            "application/xhtml+xml",
+            "application/xml",
+            "application/rss+xml",
+            "text/javascript",
+        ]) {
+            expect(isActiveContentType({ content_type })).toBe(true);
+        }
+    });
+
+    test("images, PDFs, audio and plain text are not", () => {
+        for (const content_type of ["image/png", "application/pdf", "audio/mpeg", "text/plain", "text/markdown", "", null]) {
+            expect(isActiveContentType({ content_type })).toBe(false);
+        }
+    });
+});
+
+describe("servedFileHeaders", () => {
+    test("every file is served without sniffing", () => {
+        expect(servedFileHeaders({ content_type: "application/pdf" })).toEqual({
+            "X-Content-Type-Options": "nosniff",
+        });
+    });
+
+    test("an active file is sandboxed", () => {
+        expect(servedFileHeaders({ content_type: "text/html" })).toEqual({
+            "X-Content-Type-Options": "nosniff",
+            "Content-Security-Policy": "sandbox",
+        });
+    });
+});

src/utils/served-file-headers.ts

@@ -0,0 +1,52 @@
+type Params = {
+    /**
+     * The type the file goes out with, e.g. `text/html; charset=utf-8`.
+     */
+    content_type?: string | null;
+};
+
+/**
+ * Types a browser runs as a page or script when opened. Files of these
+ * types come from users and bots (a report a bot wrote from a scraped
+ * site), and they are served from the app's own origin, so opened as they
+ * are they would run with the viewer's session.
+ */
+const ActiveContentTypes = [
+    "text/html",
+    "application/xhtml+xml",
+    "image/svg+xml",
+    "text/xml",
+    "application/xml",
+    "text/javascript",
+    "application/javascript",
+    "application/ecmascript",
+    "text/ecmascript",
+    "text/xsl",
+];
+
+/**
+ * Whether a type is one a browser would run (`ActiveContentTypes`), any
+ * `+xml` type included.
+ */
+export function isActiveContentType({ content_type }: Params) {
+    const type = (content_type || "").split(";")[0]!.trim().toLowerCase();
+
+    return ActiveContentTypes.includes(type) || type.endsWith("+xml");
+}
+
+/**
+ * The headers every user or bot file is served with: never sniffed into
+ * another type, and an active one (HTML, SVG, XML, scripts) sandboxed, so
+ * it still opens but runs no script and gets an origin of its own instead
+ * of the app's. Other types (images, PDFs, audio) are left alone, since a
+ * sandbox stops the browser's PDF viewer.
+ */
+export default function servedFileHeaders({ content_type }: Params): Record<string, string> {
+    const headers: Record<string, string> = { "X-Content-Type-Options": "nosniff" };
+
+    if (isActiveContentType({ content_type })) {
+        headers["Content-Security-Policy"] = "sandbox";
+    }
+
+    return headers;
+}

src/utils/walk-regular-files.test.ts

@@ -0,0 +1,43 @@
+import { afterAll, beforeAll, describe, expect, test } from "bun:test";
+import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import path from "node:path";
+import walkRegularFiles from "./walk-regular-files";
+
+let root = "";
+let outside = "";
+
+beforeAll(() => {
+    root = mkdtempSync(path.join(tmpdir(), "cobble-walk-"));
+    outside = mkdtempSync(path.join(tmpdir(), "cobble-walk-outside-"));
+
+    writeFileSync(path.join(outside, "secret.env"), "ENCRYPTION_KEY=x");
+    mkdirSync(path.join(outside, "folder"));
+    writeFileSync(path.join(outside, "folder", "db.sqlite"), "db");
+
+    mkdirSync(path.join(root, "reports"));
+    writeFileSync(path.join(root, "a.md"), "a");
+    writeFileSync(path.join(root, "reports", "b.md"), "b");
+    symlinkSync(path.join(outside, "secret.env"), path.join(root, "secret.env"));
+    symlinkSync(path.join(outside, "folder"), path.join(root, "linked"));
+});
+
+afterAll(() => {
+    rmSync(root, { recursive: true, force: true });
+    rmSync(outside, { recursive: true, force: true });
+});
+
+describe("walkRegularFiles", () => {
+    test("lists regular files in nested folders", () => {
+        const found = [...walkRegularFiles({ dir: root })].map((p) => path.relative(root, p)).sort();
+
+        expect(found).toEqual(["a.md", "reports/b.md"]);
+    });
+
+    test("never follows a symlink to a file or a folder", () => {
+        const found = [...walkRegularFiles({ dir: root })];
+
+        expect(found.some((p) => p.includes("secret.env"))).toBe(false);
+        expect(found.some((p) => p.includes("db.sqlite"))).toBe(false);
+    });
+});

src/utils/walk-regular-files.ts

@@ -0,0 +1,22 @@
+import { lstatSync, readdirSync } from "node:fs";
+import path from "node:path";
+
+type Params = {
+    dir: string;
+};
+
+/**
+ * Every regular file under a directory, depth first. Links are never
+ * followed: an archive unpacked from a computer can hold a symlink
+ * (`x -> /app/.env`), which would otherwise be read from the app's own
+ * disk once it lands here.
+ */
+export default function* walkRegularFiles({ dir }: Params): Generator<string> {
+    for (const name of readdirSync(dir)) {
+        const full = path.join(dir, name);
+        const stats = lstatSync(full);
+
+        if (stats.isDirectory()) yield* walkRegularFiles({ dir: full });
+        else if (stats.isFile()) yield full;
+    }
+}