From 028cde25e0a9ed693facde9f9d5f6643b322a3f5 Mon Sep 17 00:00:00 2001 From: Sivert Date: Mon, 5 Oct 2026 10:07:24 +0200 Subject: [PATCH] Revert the banner and image quarantine for now 8d7f9bd sent new banners and image uploads to quarantine/ for the image worker to clear. The worker that clears them (image-worker#38) and the client half (client#770) are not merged, so releasing server main as it stands would leave new banners and chat images unserved. It was never released. The upload-cleaning work is being planned again as one change across server, worker and client. This reverts commit 8d7f9bd. Co-Authored-By: Claude Opus 5.5 --- src/routes/bannerUpload.test.ts | 121 +++--------------------------- src/routes/uploadStorage.test.ts | 6 +- src/routes/uploads.ts | 76 +++++-------------- src/services/storeBannerUpload.ts | 15 ---- src/services/storeUploadedFile.ts | 28 +++---- 5 files changed, 42 insertions(+), 204 deletions(-) delete mode 100644 src/services/storeBannerUpload.ts diff --git a/src/routes/bannerUpload.test.ts b/src/routes/bannerUpload.test.ts index 0273c21d..c5c98dbc 100644 --- a/src/routes/bannerUpload.test.ts +++ b/src/routes/bannerUpload.test.ts @@ -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"; @@ -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. */ @@ -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 }; } @@ -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); @@ -76,7 +72,6 @@ before(async () => { const app = express(); app.use("/api/uploads", uploadsRouter); - app.use(apiErrorHandler); server = app.listen(0, "127.0.0.1"); await new Promise((resolve) => server.once("listening", () => resolve())); const { port } = server.address() as AddressInfo; @@ -84,42 +79,6 @@ before(async () => { 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((resolve) => server.close(() => resolve())); delete process.env.DATA_DIR; @@ -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()); @@ -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 () => { @@ -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; diff --git a/src/routes/uploadStorage.test.ts b/src/routes/uploadStorage.test.ts index cf83becc..6733db41 100644 --- a/src/routes/uploadStorage.test.ts +++ b/src/routes/uploadStorage.test.ts @@ -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)"], ]) { @@ -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 \}\)/); }); }); diff --git a/src/routes/uploads.ts b/src/routes/uploads.ts index 474490fc..187f3f77 100644 --- a/src/routes/uploads.ts +++ b/src/routes/uploads.ts @@ -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"; @@ -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 ``, and is sandboxed by the CSP below. That header is required. */ @@ -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. @@ -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; } @@ -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 }); @@ -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 }); @@ -222,6 +227,11 @@ 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 }); } @@ -229,42 +239,6 @@ const storeAvatarImage = (purpose: "avatar" | "group" | "webhook") => .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, @@ -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. @@ -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); diff --git a/src/services/storeBannerUpload.ts b/src/services/storeBannerUpload.ts deleted file mode 100644 index ed3f82ce..00000000 --- a/src/services/storeBannerUpload.ts +++ /dev/null @@ -1,15 +0,0 @@ -import { storeUploadedFile } from "./storeUploadedFile"; - -const bannerTypes = new Set(["image/png", "image/jpeg", "image/gif", "image/webp", "image/avif", "video/mp4"]); - -export async function storeBannerUpload(input: { - bucket: string; file: Express.Multer.File; uploadedBy: string; maxBytes: number; -}) { - const { bucket, file, uploadedBy, maxBytes } = input; - const type = file.mimetype.toLowerCase(); - if (!bannerTypes.has(type)) return { ok: false as const, status: 400, error: "invalid_file", message: "Only raster images and MP4 videos are allowed" }; - if (maxBytes > 0 && file.size > maxBytes) return { ok: false as const, status: 413, error: "file_too_large", message: "Banner exceeds the server upload limit" }; - const stored = await storeUploadedFile({ bucket, path: file.path, size: file.size, mimetype: type, - originalName: file.originalname, uploadedBy, maxBytes, sealed: false, purpose: "banner" }); - return stored.ok ? { ...stored, processing: true } : stored; -} diff --git a/src/services/storeUploadedFile.ts b/src/services/storeUploadedFile.ts index c83abc22..45d8abc1 100644 --- a/src/services/storeUploadedFile.ts +++ b/src/services/storeUploadedFile.ts @@ -1,9 +1,10 @@ +import consola from "consola"; import { readFile } from "fs/promises"; import { v4 as uuidv4 } from "uuid"; import { insertFile, insertImageJob } from "../db"; import { storageForUpload } from "../routes/uploadStorage"; -import { deleteObject, putObject } from "../storage"; +import { putObject } from "../storage"; import { validateImage } from "../utils/imageValidation"; import { sanitizeSvg } from "../utils/svgSanitize"; import { PARSE_LIMITS, readVideoDimensionsFromFile } from "../utils/videoDimensions"; @@ -13,7 +14,7 @@ import { PARSE_LIMITS, readVideoDimensionsFromFile } from "../utils/videoDimensi export const IMAGE_VALIDATION_MAX_BYTES = 64 * 1024 * 1024; export type StoreUploadedFileResult = - | { ok: true; fileId: string; key: string; processing: boolean } + | { ok: true; fileId: string; key: string } | { ok: false; status: 400 | 413; error: "invalid_file" | "file_too_large"; message: string }; function positiveInt(n: number | null | undefined): number | null { @@ -38,7 +39,6 @@ export async function storeUploadedFile(input: { claimedHeight?: number | null; /** A caller that needs the same id on every run passes its own. */ fileId?: string; - purpose?: "banner"; }): Promise { const fileId = input.fileId ?? uuidv4(); const { bucket, path, size } = input; @@ -63,10 +63,7 @@ export async function storeUploadedFile(input: { }; } - const { storedMime } = storage; - const key = input.purpose === "banner" - ? `quarantine/banners/${fileId}` - : storage.queueImageJob ? `quarantine/${storage.key}` : storage.key; + const { key, storedMime } = storage; let width: number | null = null; let height: number | null = null; @@ -94,10 +91,10 @@ export async function storeUploadedFile(input: { uploaded_by_server_user_id: input.uploadedBy, created_at: new Date(), }); - return { ok: true, fileId, key: svgKey, processing: false }; + return { ok: true, fileId, key: svgKey }; } - if (storage.validateAsImage && input.purpose !== "banner") { + if (storage.validateAsImage) { // The mime off the request is a claim, and taking it meant an SVG // carrying