Skip to content

Revert the banner and image quarantine for now #270

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sivert-io merged 1 commit into from
Oct 5, 2026
Merged

Revert the banner and image quarantine for now #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 11 additions & 110 deletions src/routes/bannerUpload.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import assert from "node:assert/strict";
import { mkdtempSync, readFileSync, rmSync } from "node:fs";
import { mkdtempSync, rmSync } from "node:fs";
import type { AddressInfo } from "node:net";
import type { Server } from "node:http";
import { tmpdir } from "node:os";
Expand All @@ -10,17 +10,15 @@ import express from "express";
import sharp from "sharp";

import { initSqlite } from "../db/sqlite/connection";
import { getFile, updateFileRecord } from "../db/sqlite/messages";
import { getFile } from "../db/sqlite/messages";
import { createRoleDefinition } from "../db/sqlite/roleDefinitions";
import { createServerConfigIfNotExists, setServerRole, updateServerConfig } from "../db/sqlite/servers";
import { createServerConfigIfNotExists, setServerRole } from "../db/sqlite/servers";
import { getUserByServerId, upsertUser } from "../db/sqlite/users";
import { unreferencedAmong } from "../jobs/mediaSweep";
import { fileReadVerdict } from "../services/fileAccess";
import { buildMemberList } from "../socket/utils/clients";
import { initStorage } from "../storage";
import { generateAccessToken, generateFileToken } from "../utils/jwt";
import { getImageJob, updateImageJobStatus } from "../db/sqlite/imageJobs";
import { apiErrorHandler } from "../utils/httpErrors";
import { generateAccessToken } from "../utils/jwt";
import { uploadsRouter } from "./uploads";

/** The member card banner: the avatar pipeline cut to 5:2. */
Expand All @@ -43,11 +41,10 @@ function auth(who: Member) {
return { Authorization: `Bearer ${generateAccessToken({ ...who, serverHost: host, tokenVersion: 0 })}` };
}

async function upload(who: Member, body: Buffer, type = "image/png", name = "banner.png", endpoint = "/banner", sealed = false) {
async function upload(who: Member, body: Buffer, type = "image/png", name = "banner.png") {
const form = new FormData();
form.append("file", new Blob([new Uint8Array(body)], { type }), name);
if (sealed) form.append("sealed", "1");
const res = await fetch(`${base}/api/uploads${endpoint}`, { method: "POST", headers: auth(who), body: form });
const res = await fetch(`${base}/api/uploads/banner`, { method: "POST", headers: auth(who), body: form });
return { status: res.status, body: (await res.json()) as Record<string, unknown> };
}

Expand All @@ -58,7 +55,6 @@ async function remove(who: Member) {

const png = (width = 1600, height = 900) =>
sharp({ create: { width, height, channels: 3, background: { r: 200, g: 60, b: 90 } } }).png().toBuffer();
const mp4 = () => readFileSync(join(__dirname, "../utils/testdata/landscape.mp4"));

const bannerOf = async (m: Member) => (await getUserByServerId(m.serverUserId))?.banner_file_id ?? null;
const rowOf = async (m: Member) => (await buildMemberList({})).find((r) => r.serverUserId === m.serverUserId);
Expand All @@ -76,50 +72,13 @@ before(async () => {

const app = express();
app.use("/api/uploads", uploadsRouter);
app.use(apiErrorHandler);
server = app.listen(0, "127.0.0.1");
await new Promise<void>((resolve) => server.once("listening", () => resolve()));
const { port } = server.address() as AddressInfo;
host = `127.0.0.1:${port}`;
base = `http://${host}`;
});

describe("shared chat upload processing", () => {
it("blocks new unencrypted pictures until the worker approves them", async () => {
const who = await member("chat-picture");
const bytes = await png(16, 16);
const response = await upload(who, bytes, "image/png", "chat.png", "");
assert.equal(response.status, 201, JSON.stringify(response.body));
const fileId = response.body.fileId as string;
assert.equal(response.body.processing, true);
assert.ok((await getFile(fileId))?.s3_key.startsWith("quarantine/uploads/"));
assert.equal((await getImageJob(fileId))?.status, "queued");
const token = generateFileToken({ ...who, serverHost: host, tokenVersion: 0 });
const url = `${base}/api/uploads/files/${fileId}?t=${encodeURIComponent(token)}`;
assert.equal((await fetch(url)).status, 503);
await updateImageJobStatus({ job_id: fileId, status: "done" });
const ready = await fetch(url);
assert.equal(ready.status, 200);
assert.deepEqual(Buffer.from(await ready.arrayBuffer()), bytes);
});

it("keeps encrypted attachments opaque and does not pass ciphertext to a decoder", async () => {
const who = await member("sealed-chat");
const bytes = Buffer.from("opaque encrypted bytes");
const response = await upload(who, bytes, "image/png", "private.png", "", true);
assert.equal(response.status, 201, JSON.stringify(response.body));
const fileId = response.body.fileId as string;
assert.equal(response.body.processing, false);
assert.equal(await getImageJob(fileId), null);
assert.equal((await getFile(fileId))?.mime, "application/octet-stream");
const token = generateFileToken({ ...who, serverHost: host, tokenVersion: 0 });
const ready = await fetch(`${base}/api/uploads/files/${fileId}?t=${encodeURIComponent(token)}`);
assert.equal(ready.status, 200);
assert.match(ready.headers.get("content-disposition") ?? "", /^attachment;/);
assert.deepEqual(Buffer.from(await ready.arrayBuffer()), bytes);
});
});

after(async () => {
await new Promise<void>((resolve) => server.close(() => resolve()));
delete process.env.DATA_DIR;
Expand All @@ -129,7 +88,7 @@ after(async () => {
});

describe("POST /api/uploads/banner", () => {
it("quarantines the picture for the worker, beside the avatar", async () => {
it("stores the picture cut to the banner box, beside the avatar", async () => {
const alice = await member("alice");
const res = await upload(alice, await png());

Expand All @@ -140,11 +99,10 @@ describe("POST /api/uploads/banner", () => {
assert.equal((await getUserByServerId(alice.serverUserId))?.avatar_file_id ?? null, null, "it became the avatar");

const file = await getFile(fileId);
assert.equal(file?.mime, "image/png");
assert.equal(file?.width, null);
assert.equal(file?.height, null);
assert.ok(file?.s3_key.startsWith("quarantine/banners/"));
assert.equal((await getImageJob(fileId))?.status, "queued");
assert.equal(file?.mime, "image/avif");
assert.equal(file?.width, 960);
assert.equal(file?.height, 384);
assert.ok(file?.s3_key.startsWith("banners/"));
});

it("is refused to a member who may not upload, and nothing is stored", async () => {
Expand All @@ -162,63 +120,6 @@ describe("POST /api/uploads/banner", () => {
assert.equal(await bannerOf(bob), null);
});

it("stores a readable MP4 without flattening its animation", async () => {
const player = await member("player");
const res = await upload(player, mp4(), "video/mp4", "loop.mp4");
assert.equal(res.status, 201, JSON.stringify(res.body));

const file = await getFile(res.body.bannerFileId as string);
assert.equal(file?.mime, "video/mp4");
assert.equal(file?.width, 160);
assert.equal(file?.height, 90);
assert.equal((await getImageJob(res.body.bannerFileId as string))?.status, "queued");
});

it("keeps bytes merely labelled as MP4 unreadable until the worker checks them", async () => {
const poser = await member("poser");
const res = await upload(poser, Buffer.from("not a video"), "video/mp4", "fake.mp4");
assert.equal(res.status, 201);
const fileId = res.body.bannerFileId as string;
const token = generateFileToken({ ...poser, serverHost: host, tokenVersion: 0 });
const url = `${base}/api/uploads/files/${fileId}?t=${encodeURIComponent(token)}`;
const pending = await fetch(url);
assert.equal(pending.status, 503);
assert.equal(pending.headers.get("cache-control"), "no-store");
await updateImageJobStatus({ job_id: fileId, status: "error", error_message: "invalid video" });
assert.equal((await fetch(url)).status, 422);
});

it("serves a video only after a successful worker verdict, including range reads", async () => {
const verified = await member("verified");
const res = await upload(verified, mp4(), "video/mp4", "loop.mp4");
const fileId = res.body.bannerFileId as string;
const token = generateFileToken({ ...verified, serverHost: host, tokenVersion: 0 });
const url = `${base}/api/uploads/files/${fileId}?t=${encodeURIComponent(token)}`;
assert.equal((await fetch(url, { method: "HEAD" })).status, 503);
assert.equal((await fetch(`${url}&thumb=1`)).status, 503);
await updateImageJobStatus({ job_id: fileId, status: "done" });
assert.equal((await fetch(url)).status, 422, "a done job without a decoded poster must remain blocked");
await updateFileRecord(fileId, { thumbnail_key: `thumbnails/${fileId}.jpg` });
const ready = await fetch(url, { headers: { Range: "bytes=0-15" } });
assert.equal(ready.status, 206);
assert.equal((await ready.arrayBuffer()).byteLength, 16);
});

it("enforces the configured upload limit on both pictures and videos", async () => {
const limited = await member("limited");
await updateServerConfig({ uploadMaxBytes: 512 });
try {
for (const [bytes, type] of [[await png(), "image/png"], [mp4(), "video/mp4"]] as const) {
const res = await upload(limited, bytes, type);
assert.equal(res.status, 413, JSON.stringify(res.body));
assert.equal(res.body.error, "file_too_large");
assert.equal(await bannerOf(limited), null);
}
} finally {
await updateServerConfig({ uploadMaxBytes: null });
}
});

it("deletes the old banner when it is replaced", async () => {
const carol = await member("carol");
const first = (await upload(carol, await png())).body.bannerFileId as string;
Expand Down
6 changes: 3 additions & 3 deletions src/routes/uploadStorage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ describe("the route uses it", () => {
// sharp, the worker and the SVG sanitiser would each be handed it.
for (const [what, gate] of [
["the SVG sanitiser", "if (storage.treatAsSvg)"],
["image validation", "if (storage.validateAsImage && input.purpose !== \"banner\")"],
["image validation", "if (storage.validateAsImage)"],
["the video container parser", "if (storage.measureAsVideo)"],
["the image worker", "if (storage.queueImageJob)"],
]) {
Expand All @@ -187,7 +187,7 @@ describe("the route uses it", () => {
assert.doesNotMatch(service, /ffmpeg|child_process|execFile|spawn\(/);
});

it("reports whether the shared worker must finish before media can be read", () => {
assert.match(route, /res\.status\(201\)\.json\(\{ fileId: stored\.fileId, key: stored\.key, thumbnailKey: null, processing: stored\.processing \}\)/);
it("answers the uploader the same whether or not a poster ever comes", () => {
assert.match(route, /res\.status\(201\)\.json\(\{ fileId: stored\.fileId, key: stored\.key, thumbnailKey: null \}\)/);
});
});
76 changes: 18 additions & 58 deletions src/routes/uploads.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import mime from "mime-types";
import { unlink } from "fs/promises";
import { putObject, getObject } from "../storage";
import { insertFile, getFile, updateUserAvatar, setUserAvatar, setUserBanner, getServerConfig, getUserByServerId, DEFAULT_AVATAR_MAX_BYTES, DEFAULT_UPLOAD_MAX_BYTES } from "../db";
import { BANNER_BOX } from "../constants/media";
import { deleteUnreferencedFiles } from "../jobs/mediaSweep";
import { isSealedUpload } from "./uploadStorage";
import { requireBearerToken } from "../middleware/requireBearerToken";
Expand All @@ -19,8 +20,6 @@ import { sendStoredBody } from "../utils/sendStoredBody";
import { ensurePermission } from "../middleware/requirePermission";
import { sanitizeSvg } from "../utils/svgSanitize";
import { storeUploadedFile } from "../services/storeUploadedFile";
import { storeBannerUpload } from "../services/storeBannerUpload";
import { getImageJob } from "../db";

/** SVG is on this list only because it has been through sanitizeSvg(), is drawn
through `<img>`, and is sandboxed by the CSP below. That header is required. */
Expand Down Expand Up @@ -131,7 +130,7 @@ uploadsRouter.post(
res.status(stored.status).json({ error: stored.error, message: stored.message });
return;
}
res.status(201).json({ fileId: stored.fileId, key: stored.key, thumbnailKey: null, processing: stored.processing });
res.status(201).json({ fileId: stored.fileId, key: stored.key, thumbnailKey: null });
})
// Every exit path, including the early returns and anything that threw:
// multer's temp file is ours and nothing else removes it.
Expand All @@ -140,15 +139,15 @@ uploadsRouter.post(
},
);

/** One pipeline for avatar-shaped pictures. Only an avatar is put on the
/** One pipeline for every picture. Only an avatar or a banner is put on the
uploader's own row; a group picture doing that was GRYT-1182. */
const storeAvatarImage = (purpose: "avatar" | "group" | "webhook") =>
const storeAvatarImage = (purpose: "avatar" | "group" | "webhook" | "banner") =>
(req: Request, res: Response, next: NextFunction): void => {
const file = req.file;
if (!file) { res.status(400).json({ error: "file_required", message: "file is required" }); return; }
if (!(file.mimetype || "").startsWith("image/")) { res.status(400).json({ error: "invalid_file", message: "Only image files are allowed" }); return; }

const what = { avatar: "Avatar", group: "Group picture", webhook: "Webhook avatar" }[purpose];
const what = { avatar: "Avatar", group: "Group picture", webhook: "Webhook avatar", banner: "Banner" }[purpose];
const disableS3 = (process.env.DISABLE_S3 || "").toLowerCase() === "true";
if (disableS3) { res.status(503).json({ error: "s3_disabled", message: `S3 is disabled (DISABLE_S3=true). ${what} upload is unavailable.` }); return; }

Expand All @@ -175,6 +174,11 @@ const storeAvatarImage = (purpose: "avatar" | "group" | "webhook") =>
// SVG never reaches sharp. Stored as the sanitised vector; see
// svgSanitize.ts for why that is enough.
if ((file.mimetype || "").toLowerCase() === "image/svg+xml") {
// A vector is not cut to the 5:2 box, so a banner is always a raster.
if (purpose === "banner") {
res.status(400).json({ error: "invalid_file", message: "A banner can't be an SVG." });
return;
}
const svg = sanitizeSvg(file.buffer);
if (!svg.valid) {
res.status(400).json({ error: "invalid_file", message: svg.reason });
Expand Down Expand Up @@ -213,6 +217,7 @@ const storeAvatarImage = (purpose: "avatar" | "group" | "webhook") =>
uploadedBy: serverUserId,
maxBytes,
what,
...(purpose === "banner" ? { box: BANNER_BOX, prefix: "banners" } : {}),
});
if (!stored.ok) {
res.status(stored.status).json({ error: stored.error, message: stored.message });
Expand All @@ -222,49 +227,18 @@ const storeAvatarImage = (purpose: "avatar" | "group" | "webhook") =>
if (purpose === "avatar") {
await updateUserAvatar(serverUserId, stored.fileId);
res.status(201).json({ avatarFileId: stored.fileId, processing: stored.processing });
} else if (purpose === "banner") {
const previous = (await getUserByServerId(serverUserId))?.banner_file_id ?? null;
await setUserBanner(serverUserId, stored.fileId);
if (previous && previous !== stored.fileId) await deleteUnreferencedFiles([previous]);
res.status(201).json({ bannerFileId: stored.fileId, processing: stored.processing });
} else {
res.status(201).json({ fileId: stored.fileId, processing: stored.processing });
}
})
.catch(next);
};

const storeBannerMedia = (req: Request, res: Response, next: NextFunction): void => {
const file = req.file;
if (!file) { res.status(400).json({ error: "file_required", message: "file is required" }); return; }
const type = (file.mimetype || "").toLowerCase();
if (!type.startsWith("image/") && type !== "video/mp4") {
void discardTemp(file);
res.status(400).json({ error: "invalid_file", message: "Only images and MP4 videos are allowed" });
return;
}

const bucket = process.env.S3_BUCKET as string;
const serverUserId = req.tokenPayload?.serverUserId;
const disableS3 = (process.env.DISABLE_S3 || "").toLowerCase() === "true";
if (disableS3) { void discardTemp(file); res.status(503).json({ error: "s3_disabled", message: "S3 is disabled (DISABLE_S3=true). Banner upload is unavailable." }); return; }
if (!bucket) { void discardTemp(file); res.status(500).json({ error: "s3_not_configured", message: "S3_BUCKET not configured" }); return; }
if (!serverUserId) { void discardTemp(file); res.status(401).json({ error: "auth_required" }); return; }

Promise.resolve()
.then(async () => {
const cfg = await getServerConfig().catch(() => null);
const maxBytes = typeof cfg?.upload_max_bytes === "number" ? cfg.upload_max_bytes : DEFAULT_UPLOAD_MAX_BYTES;
const stored = await storeBannerUpload({ bucket, file, uploadedBy: serverUserId, maxBytes });

if (!stored.ok) {
res.status(stored.status).json({ error: stored.error, message: stored.message });
return;
}
const previous = (await getUserByServerId(serverUserId))?.banner_file_id ?? null;
await setUserBanner(serverUserId, stored.fileId);
if (previous && previous !== stored.fileId) await deleteUnreferencedFiles([previous]);
res.status(201).json({ bannerFileId: stored.fileId, processing: stored.processing, mime: type });
})
.finally(() => discardTemp(file))
.catch(next);
};

uploadsRouter.post(
"/avatar",
requireBearerToken,
Expand All @@ -287,8 +261,8 @@ uploadsRouter.post(
.then((ok) => { if (ok) next(); })
.catch(next);
},
uploadToDisk("file"),
storeBannerMedia,
uploadAvatarToMemory("file"),
storeAvatarImage("banner"),
);

// Ungated, unlike DELETE /avatar: losing the upload permission must not strand a banner.
Expand Down Expand Up @@ -444,20 +418,6 @@ uploadsRouter.get(
const fileMeta = verdict === "allowed" ? await getFile(fileId) : null;
if (!fileMeta) { res.status(404).json({ error: "File not found" }); return; }

// New media stays unreadable when the worker is absent or fails.
const job = await getImageJob(fileId);
if (job || fileMeta.s3_key.startsWith("quarantine/") || fileMeta.s3_key.startsWith("banners/verified/")) {
if (job?.status !== "done") {
res.setHeader("Cache-Control", "no-store");
res.status(job?.status === "error" ? 422 : 503).json({ error: "media_not_ready" });
return;
}
if (fileMeta.mime?.startsWith("video/") && !fileMeta.thumbnail_key) {
res.status(422).json({ error: "media_not_validated" });
return;
}
}

const useThumb = req.query.thumb === "1" && fileMeta.thumbnail_key;
const s3Key = useThumb ? fileMeta.thumbnail_key! : fileMeta.s3_key;
const totalSize = useThumb ? null : (fileMeta.size ?? null);
Expand Down
15 changes: 0 additions & 15 deletions src/services/storeBannerUpload.ts
View file
Open in desktop

This file was deleted.

Loading
Loading