vincent/gerbeur
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

v3: fix the privacy and security findings of the comment pass — private dumps' broadcasts, mentions and attachments reached people who can't see them, and a few smaller holes. A dump's every write now goes through ws-service's broadcastDumpChange(before, after), which decides the audience: a private dump's creation, edits and deletion reach its owner's sockets alone (its deletion id went to everyone before), going private is dump_deleted for everyone else and dump_updated for the owner (whose other tabs dropped it), going public dump_created for everyone else. Mentions in a private dump, a comment on one, or a private playlist's description notify no one until it goes public (mentions are deduplicated per context, so going public notifies them once), and a moderator's edit of a comment, chat message or playlist mentions in the author's name. A link preview of a private playlist is refused, as one of a private dump already was: the preview is stored in the linking dump, which others may see. Attachments record the type of the resource that links them (migration 0018 backfills it); one linked by a private dump or playlist, or a comment on a private dump, is served only with its owner's media token, which Markdown now adds to attachment URLs, and one whose resource is gone answers 404 and is purged at startup. The Bandcamp provider claimed bandcamp.com.evil.example (the pattern wasn't anchored) and now reuses bandcamp-tralbum's anchored one; the YouTube provider took any site's ?list= for a YouTube playlist and missed m., music., youtube-nocookie and /live/ links, and now shares canonical-url's rules. Sign-in does the same scrypt work for an unknown username as for a wrong password, so timing no longer tells which exist. Usernames and emails are unique regardless of case (migration 0017, which refuses to run over existing clashes and names them): "Bob" and "bob" could both sign up and a mention picked one, emails had no constraint, and a reset request in another case silently sent nothing. The Open Graph tags escape the site name and build their URLs on PUBLIC_URL and FRONTEND_URL instead of the request's origin, which a proxy may rewrite.

Browse Source 
Error statuses follow one rule: APIException takes a status and derives the code from it (BAD_REQUEST 400, UNAUTHORIZED 401, FORBIDDEN 403, NOT_FOUND 404, CONFLICT 409, RATE_LIMITED 429, SERVER_ERROR 500, BAD_GATEWAY 502), so a code can't contradict its status. That fixed real bugs: a wrong current password on change-password answered 401, on which authFetch signs the user out; editing or deleting someone else's comment or chat message answered 401 rather than 403; one invalid dump edit answered 422. A taken username is 409, like a taken email.

Along the way: the database is created from schema.sql by connection.ts when the file doesn't exist, so every task works on a fresh checkout (dev and serve never ran db/init.ts, which is gone); the startup purge compared ISO dates with SQLite's datetime() as text and kept anything from the current UTC day, and now compares ISO cutoffs; the seeded admin's hash comes from lib/password.ts (split out of jwt.ts, which needs the JWT secret) rather than a copy of its format; avatar, thumbnail and playlist-cover uploads go through replaceUploadedFile, which writes beside the old file and renames over it before the record announces it (a failed avatar write lost the old avatar; a thumbnail was announced before it existed); upload.ts derives its size message from MAX_IMAGE_SIZE_BYTES and stops re-exporting the upload folders; ALLOWED_IMAGE_MIMES, which duplicated detectImageMime, is gone; the SPA fallback no longer calls next() after answering; a reply must answer a comment on the same dump (a foreign or unknown parent was a 500 or a cross-dump thread). 211 backend and 628 frontend tests pass.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
khannurien
2026-09-29 15:19:07 +00:00
parent 0d4e344d77
commit 14e3ab125b
69 changed files with 1011 additions and 1069 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
api/db/connection.ts
View File

@@ -1,7 +1,8 @@
/**
* Opens the database at {@link DB_PATH} when first imported, applies the
* pending migrations and runs the startup tasks before any importer runs. Only
* the services import it.
* Opens the database at {@link DB_PATH} when first imported, creating it from
* schema.sql if the file doesn't exist, then applies the pending migrations
* and runs the startup tasks before any importer runs. Only the services
* import it.
*
* @module
*/
@@ -11,11 +12,20 @@ import { DB_PATH } from "../config.ts";
import { runMigrations } from "./migrate.ts";
import { runStartupTasks } from "./startup.ts";
const isNew = await Deno.stat(DB_PATH).then(() => false, () => true);
/**
* The server's only connection. Its queries are synchronous, so each one
* blocks the event loop while it runs.
*/
export const db: DatabaseSync = new DatabaseSync(DB_PATH);
// schema.sql holds every migration's change, so on a new database each
// migration below finds its work done and is only recorded.
if (isNew) {
db.exec(
await Deno.readTextFile(new URL("./schema.sql", import.meta.url)),
);
}
// WAL lets readers run alongside a writer, and with synchronous = NORMAL a
// commit doesn't wait on an fsync: a power loss can lose the last commits but
// never corrupts the file. busy_timeout makes another connection to the file

23
api/db/init.ts
View File

@@ -1,23 +0,0 @@
/**
* Creates the database from schema.sql when {@link DB_PATH} doesn't exist yet,
* and leaves an existing one alone. `deno task start` runs it before the
* server, which then runs every migration on the new file: schema.sql already
* holds their changes, so each one finds nothing to do.
*
* @module
*/
import { DatabaseSync } from "node:sqlite";
import { DB_PATH } from "../config.ts";
try {
await Deno.stat(DB_PATH);
console.log("Database already exists, skipping initialization.");
} catch {
console.log("Initializing database from schema...");
const schema = Deno.readTextFileSync("api/db/schema.sql");
const db = new DatabaseSync(DB_PATH);
db.exec(schema);
db.close();
console.log("Database initialized.");
}

11
api/db/migrate.ts
View File

@@ -23,6 +23,8 @@ import { up as up0013DumpUrlCanonical } from "./migrations/0013_dump_url_canonic
import { up as up0014ChatReadState } from "./migrations/0014_chat_read_state.ts";
import { up as up0015TokenRevocation } from "./migrations/0015_token_revocation.ts";
import { up as up0016PerformanceIndexes } from "./migrations/0016_performance_indexes.ts";
import { up as up0017CaseInsensitiveUsers } from "./migrations/0017_case_insensitive_users.ts";
import { up as up0018AttachmentResourceType } from "./migrations/0018_attachment_resource_type.ts";
interface Migration {
name: string;
@@ -32,8 +34,8 @@ interface Migration {
/**
* Every migration, in the order they run. A new one goes at the end, along
* with the same change to schema.sql, and a shipped one is never edited or
* moved. A database made by init.ts records none of them, so each `up` must
* find its change already made and leave it be.
* moved. A database connection.ts made from schema.sql records none of them,
* so each `up` must find its change already made and leave it be.
*/
const MIGRATIONS: Migration[] = [
{ name: "0001_comment_likes", up: up0001CommentLikes },
@@ -55,6 +57,11 @@ const MIGRATIONS: Migration[] = [
{ name: "0014_chat_read_state", up: up0014ChatReadState },
{ name: "0015_token_revocation", up: up0015TokenRevocation },
{ name: "0016_performance_indexes", up: up0016PerformanceIndexes },
{ name: "0017_case_insensitive_users", up: up0017CaseInsensitiveUsers },
{
name: "0018_attachment_resource_type",
up: up0018AttachmentResourceType,
},
];
/**

36
api/db/migrations/0017_case_insensitive_users.ts Normal file
View File

@@ -0,0 +1,36 @@
/**
* Makes usernames and emails unique regardless of case. Usernames were unique
* only as typed, so "Bob" and "bob" could both sign up, and a mention, which
* looks names up in lower case, then reached one of them at random. Emails
* had no constraint at all: two sign-ups racing past the service's check could
* share an address, and password resets are looked up by address.
*
* @module
*/
import type { DatabaseSync } from "node:sqlite";
/**
* Run once by `runMigrations`. It refuses to run, naming the clashes, when two
* accounts already share a name or an address in some case: which one keeps
* it is for whoever runs the site to decide, by editing one of them.
*/
export function up(db: DatabaseSync): void {
for (const column of ["username", "email"]) {
const clashes = db.prepare(
`SELECT lower(${column}) AS value FROM users GROUP BY lower(${column}) HAVING COUNT(*) > 1;`,
).all() as { value: string }[];
if (clashes.length > 0) {
throw new Error(
`Several accounts share a ${column} in different cases: ${
clashes.map((c) => c.value).join(", ")
}. Change all but one of each, then restart.`,
);
}
}
db.exec(`
DROP INDEX IF EXISTS idx_users_username_lower;
CREATE UNIQUE INDEX IF NOT EXISTS idx_users_username_lower ON users(lower(username));
CREATE UNIQUE INDEX IF NOT EXISTS idx_users_email_lower ON users(lower(email));
`);
}

34
api/db/migrations/0018_attachment_resource_type.ts Normal file
View File

@@ -0,0 +1,34 @@
/**
* Adds `attachments.resource_type`, the kind of thing an attachment's
* `resource_id` names. Without it, an attachment couldn't tell whose it was:
* one pasted into a private dump was served to anyone with its URL, and one
* whose dump, comment or message was deleted was kept for good.
*
* @module
*/
import type { DatabaseSync } from "node:sqlite";
/**
* Run once by `runMigrations`. Existing bound attachments get the type of the
* table that holds their resource; one whose resource is gone gets none, and
* the startup purge deletes it. A database made from schema.sql already has
* the column.
*/
export function up(db: DatabaseSync): void {
const cols = db.prepare(`PRAGMA table_info(attachments);`).all() as {
name: string;
}[];
if (cols.some((c) => c.name === "resource_type")) return;
db.exec(`
ALTER TABLE attachments ADD COLUMN resource_type TEXT;
UPDATE attachments SET resource_type = CASE
WHEN resource_id IN (SELECT id FROM dumps) THEN 'dump'
WHEN resource_id IN (SELECT id FROM comments) THEN 'comment'
WHEN resource_id IN (SELECT id FROM playlists) THEN 'playlist'
WHEN resource_id IN (SELECT id FROM chat_messages) THEN 'chat'
WHEN resource_id IN (SELECT id FROM users) THEN 'user'
END
WHERE resource_id IS NOT NULL;
`);
}

6
api/db/schema.sql
View File

@@ -175,13 +175,15 @@ CREATE TABLE invites (
CREATE INDEX idx_invites_inviter ON invites(inviter_id);
CREATE INDEX idx_users_invited_by ON users(invited_by);
CREATE INDEX idx_users_username_lower ON users(lower(username));
CREATE UNIQUE INDEX idx_users_username_lower ON users(lower(username));
CREATE UNIQUE INDEX idx_users_email_lower ON users(lower(email));
CREATE TABLE attachments (
id TEXT PRIMARY KEY,
resource_id TEXT,
mime TEXT NOT NULL,
created_at TEXT NOT NULL
created_at TEXT NOT NULL,
resource_type TEXT
);
CREATE INDEX idx_attachments_resource ON attachments(resource_id);

54
api/db/startup.ts
View File

@@ -5,41 +5,61 @@
* @module
*/
import { randomBytes, scryptSync } from "node:crypto";
import type { DatabaseSync } from "node:sqlite";
import {
ATTACHMENTS_DIR,
ORPHANED_ATTACHMENTS_RETENTION_HOURS,
UNUSED_INVITES_RETENTION_DAYS,
} from "../config.ts";
import { hashPassword } from "../lib/password.ts";
/** Returns the time `ms` ago as an ISO string, the format dates are stored in. */
function isoAgo(ms: number): string {
return new Date(Date.now() - ms).toISOString();
}
/**
* Runs after the migrations, each time the server starts. It deletes used or
* expired reset tokens, invites past their validity and attachments nothing
* links to. On an instance with no users, it creates the user `admin` with the
* expired reset tokens, invites past their validity, and attachments nothing
* links to or whose resource is gone. On an instance with no users, it creates the user `admin` with the
* password `admin`, which whoever installs it must change.
*/
export async function runStartupTasks(db: DatabaseSync): Promise<void> {
// Dates are stored as ISO strings and compared as text, so the cutoffs are
// ISO strings too. SQLite's datetime() has a space where ISO has a `T`.
db.prepare(
`DELETE FROM password_reset_tokens WHERE expires_at < datetime('now') OR used_at IS NOT NULL;`,
).run();
`DELETE FROM password_reset_tokens WHERE expires_at < ? OR used_at IS NOT NULL;`,
).run(isoAgo(0));
db.prepare(
`DELETE FROM invites WHERE used_at IS NULL AND created_at < datetime('now', '-${UNUSED_INVITES_RETENTION_DAYS} days');`,
).run();
`DELETE FROM invites WHERE used_at IS NULL AND created_at < ?;`,
).run(isoAgo(UNUSED_INVITES_RETENTION_DAYS * 86_400_000));
// An image is uploaded as soon as it's pasted, and linked only when the text
// it's pasted into is saved, so a fresh one is left alone.
// it's pasted into is saved, so a fresh unlinked one is left alone. One whose
// dump, comment, playlist, message or user is gone goes whatever its age.
const attachmentCutoff = isoAgo(
ORPHANED_ATTACHMENTS_RETENTION_HOURS * 3_600_000,
);
const ORPHANED = `
(resource_id IS NULL AND created_at < ?)
OR (resource_id IS NOT NULL AND NOT CASE resource_type
WHEN 'dump' THEN EXISTS (SELECT 1 FROM dumps WHERE id = resource_id)
WHEN 'comment' THEN EXISTS (SELECT 1 FROM comments WHERE id = resource_id AND deleted = 0)
WHEN 'playlist' THEN EXISTS (SELECT 1 FROM playlists WHERE id = resource_id)
WHEN 'chat' THEN EXISTS (SELECT 1 FROM chat_messages WHERE id = resource_id)
WHEN 'user' THEN EXISTS (SELECT 1 FROM users WHERE id = resource_id)
ELSE 0
END)`;
const orphanedAttachments = db.prepare(
`SELECT id FROM attachments WHERE resource_id IS NULL AND created_at < datetime('now', '-${ORPHANED_ATTACHMENTS_RETENTION_HOURS} hour');`,
).all() as { id: string }[];
`SELECT id FROM attachments WHERE ${ORPHANED};`,
).all(attachmentCutoff) as { id: string }[];
if (orphanedAttachments.length > 0) {
for (const { id } of orphanedAttachments) {
await Deno.remove(`${ATTACHMENTS_DIR}/${id}`).catch(() => {});
}
db.prepare(
`DELETE FROM attachments WHERE resource_id IS NULL AND created_at < datetime('now', '-${ORPHANED_ATTACHMENTS_RETENTION_HOURS} hour');`,
).run();
db.prepare(`DELETE FROM attachments WHERE ${ORPHANED};`)
.run(attachmentCutoff);
}
// Registration needs an invite, so the first user can only come from here.
@@ -47,13 +67,9 @@ export async function runStartupTasks(db: DatabaseSync): Promise<void> {
count: number;
};
if (userCount.count === 0) {
// The format of hashPassword in lib/jwt.ts, which verifies it.
const salt = randomBytes(16).toString("hex");
const hash = scryptSync("admin", salt, 64).toString("hex");
const passwordHash = `${hash}.${salt}`;
db.prepare(
`INSERT INTO users (id, username, password_hash, role, created_at, email) VALUES (?, 'admin', ?, 'admin', datetime('now'), 'admin@localhost')`,
).run(crypto.randomUUID(), passwordHash);
`INSERT INTO users (id, username, password_hash, role, created_at, email) VALUES (?, 'admin', ?, 'admin', ?, 'admin@localhost')`,
).run(crypto.randomUUID(), await hashPassword("admin"), isoAgo(0));
console.log(
"Created default admin user (username: admin, password: admin)",
);