From cb5adfe996040417451f31e5f87eb7c84dfe5807 Mon Sep 17 00:00:00 2001 From: Sivert Date: Tue, 15 Sep 2026 08:58:38 +0200 Subject: [PATCH] Choosing a group picture replaced your own avatar (GRYT-1182) Group pictures went through POST /api/uploads/avatar, which always writes the uploader's avatar_file_id. So picking a picture for a group also made it your avatar on that server. There's a separate POST /api/uploads/group-icon now. It runs the same pipeline as the avatar route (size limit, SVG sanitising, sharp, the animated resize) but never touches the user row, and it answers { fileId, processing }. It's gated on send_direct_messages, which is what dm:group:create and dm:group:update check. The media sweep only kept files that a message or a user avatar pointed at. Group pictures got away with that because they were also somebody's avatar. Once they aren't, the sweep deletes them 30 minutes after upload, so it keeps anything a group wears as its icon too. Co-Authored-By: Claude Opus 5 --- src/db/sqlite/conversations.ts | 9 ++ src/jobs/mediaSweep.ts | 27 +++--- src/routes/groupIconUpload.test.ts | 138 +++++++++++++++++++++++++++++ src/routes/uploadAuthOrder.test.ts | 2 +- src/routes/uploadStorage.test.ts | 3 +- src/routes/uploads.ts | 59 ++++++++---- 6 files changed, 205 insertions(+), 33 deletions(-) create mode 100644 src/routes/groupIconUpload.test.ts diff --git a/src/db/sqlite/conversations.ts b/src/db/sqlite/conversations.ts index d923556b..df4f46ec 100644 --- a/src/db/sqlite/conversations.ts +++ b/src/db/sqlite/conversations.ts @@ -182,6 +182,15 @@ export async function setConversationIcon( ).run(fileId, conversationId); } +/** For the media sweep, which would otherwise delete a group's picture. */ +export async function getAllGroupIconFileIds(): Promise> { + const db = getSqliteDb(); + const rows = db.prepare( + `SELECT icon_file_id FROM conversations WHERE kind = 'group' AND icon_file_id IS NOT NULL`, + ).all() as { icon_file_id: string }[]; + return new Set(rows.map((r) => r.icon_file_id)); +} + /** Name a group, or clear the name so it goes back to reading off its members. */ export async function setConversationName( conversationId: string, diff --git a/src/jobs/mediaSweep.ts b/src/jobs/mediaSweep.ts index 4f2333dc..0b937b94 100644 --- a/src/jobs/mediaSweep.ts +++ b/src/jobs/mediaSweep.ts @@ -3,6 +3,7 @@ import { deleteFileRecord, getAllAvatarFileIds, getAllFileRecords, + getAllGroupIconFileIds, getAllReferencedAttachmentIds, } from "../db"; import { deleteObject } from "../storage"; @@ -23,13 +24,7 @@ export async function runMediaSweep(): Promise<{ deleted: number; errors: number let deleted = 0; let errors = 0; - const [allFiles, referencedByMessages, referencedByAvatars] = await Promise.all([ - getAllFileRecords(), - getAllReferencedAttachmentIds(), - getAllAvatarFileIds(), - ]); - - const referencedIds = new Set([...referencedByMessages, ...referencedByAvatars]); + const [allFiles, referencedIds] = await Promise.all([getAllFileRecords(), referencedFileIds()]); const orphaned = allFiles.filter((f) => { if (referencedIds.has(f.file_id)) return false; @@ -62,16 +57,22 @@ export async function runMediaSweep(): Promise<{ deleted: number; errors: number return { deleted, errors }; } -/** Which of `fileIds` nothing points at any more. Reads the same two sources as +/** One list for both deleters, so neither can remove what the other keeps. */ +async function referencedFileIds(): Promise> { + const sources = await Promise.all([ + getAllReferencedAttachmentIds(), + getAllAvatarFileIds(), + getAllGroupIconFileIds(), + ]); + return new Set(sources.flatMap((ids) => [...ids])); +} + +/** Which of `fileIds` nothing points at any more. Reads the same sources as the periodic sweep, so it cannot select something the sweep would keep. */ export async function unreferencedAmong(fileIds: string[]): Promise { if (fileIds.length === 0) return []; - const [referencedByMessages, referencedByAvatars] = await Promise.all([ - getAllReferencedAttachmentIds(), - getAllAvatarFileIds(), - ]); - const referenced = new Set([...referencedByMessages, ...referencedByAvatars]); + const referenced = await referencedFileIds(); return fileIds.filter((id) => !referenced.has(id)); } diff --git a/src/routes/groupIconUpload.test.ts b/src/routes/groupIconUpload.test.ts new file mode 100644 index 00000000..ad1d9d2d --- /dev/null +++ b/src/routes/groupIconUpload.test.ts @@ -0,0 +1,138 @@ +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync } from "node:fs"; +import type { AddressInfo } from "node:net"; +import type { Server } from "node:http"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { after, before, describe, it } from "node:test"; + +import express from "express"; +import sharp from "sharp"; + +import { initSqlite } from "../db/sqlite/connection"; +import { createGroupConversation, setConversationIcon } from "../db/sqlite/conversations"; +import { getFile, getFileOwnership } from "../db/sqlite/messages"; +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 { initStorage } from "../storage"; +import { generateAccessToken } from "../utils/jwt"; +import { uploadsRouter } from "./uploads"; + +/** + * Choosing a group's picture went through the avatar route, which also made it + * the uploader's own avatar (GRYT-1182). + */ + +let dir: string; +let server: Server; +let base = ""; +let host = ""; + +interface Member { serverUserId: string; grytUserId: string; nickname: string } + +async function member(nickname: string): Promise { + const grytUserId = `acct-${nickname}`; + const user = await upsertUser(grytUserId, nickname); + await setServerRole(user.server_user_id, "member"); + return { serverUserId: user.server_user_id, grytUserId, nickname }; +} + +async function upload(path: string, who: Member, body: Buffer, type: string, name: string) { + const form = new FormData(); + form.append("file", new Blob([new Uint8Array(body)], { type }), name); + const token = generateAccessToken({ ...who, serverHost: host, tokenVersion: 0 }); + const res = await fetch(`${base}/api/uploads/${path}`, { + method: "POST", + headers: { Authorization: `Bearer ${token}` }, + body: form, + }); + return { status: res.status, body: (await res.json()) as Record }; +} + +const png = () => + sharp({ create: { width: 64, height: 64, channels: 3, background: { r: 20, g: 160, b: 140 } } }).png().toBuffer(); + +const SVG = Buffer.from(''); + +before(async () => { + dir = mkdtempSync(join(tmpdir(), "gryt-groupicon-")); + process.env.DATA_DIR = dir; + process.env.S3_BUCKET = "gryt-test"; + process.env.STORAGE_BACKEND = "filesystem"; + await initSqlite(); + initStorage(); + await createServerConfigIfNotExists(); + + const app = express(); + app.use("/api/uploads", uploadsRouter); + server = app.listen(0, "127.0.0.1"); + await new Promise((resolve) => server.once("listening", () => resolve())); + const { port } = server.address() as AddressInfo; + host = `127.0.0.1:${port}`; + base = `http://${host}`; +}); + +after(async () => { + await new Promise((resolve) => server.close(() => resolve())); + delete process.env.DATA_DIR; + delete process.env.S3_BUCKET; + delete process.env.STORAGE_BACKEND; + rmSync(dir, { recursive: true, force: true }); +}); + +describe("POST /api/uploads/group-icon", () => { + it("stores the picture without touching the uploader's avatar", async () => { + const alice = await member("alice"); + const res = await upload("group-icon", alice, await png(), "image/png", "group.png"); + + assert.equal(res.status, 201, JSON.stringify(res.body)); + const fileId = res.body.fileId as string; + assert.ok(fileId, "no fileId in the reply"); + assert.equal(res.body.avatarFileId, undefined); + + assert.equal((await getUserByServerId(alice.serverUserId))?.avatar_file_id ?? null, null); + const row = await getFile(fileId); + assert.equal(row?.mime, "image/avif", "the picture skipped the avatar processing"); + assert.equal((await getFileOwnership(fileId))?.uploadedBy, alice.serverUserId); + // What `dm:group:update` checks before it will set the icon. + assert.equal(await fileReadVerdict(fileId, alice.serverUserId, alice.grytUserId), "allowed"); + }); + + it("leaves the avatar alone on the SVG path too", async () => { + const bob = await member("bob"); + const res = await upload("group-icon", bob, SVG, "image/svg+xml", "group.svg"); + + assert.equal(res.status, 200, JSON.stringify(res.body)); + assert.ok(res.body.fileId); + assert.equal((await getUserByServerId(bob.serverUserId))?.avatar_file_id ?? null, null); + }); + + it("refuses what the avatar route refuses", async () => { + const carol = await member("carol"); + const res = await upload("group-icon", carol, Buffer.from("not a picture"), "text/plain", "x.txt"); + assert.equal(res.status, 400); + }); + + it("is still what sets an avatar on the avatar route", async () => { + const dave = await member("dave"); + const res = await upload("avatar", dave, await png(), "image/png", "me.png"); + + assert.equal(res.status, 201, JSON.stringify(res.body)); + assert.equal((await getUserByServerId(dave.serverUserId))?.avatar_file_id, res.body.avatarFileId); + }); + + it("is kept by the media sweep once a group wears it", async () => { + const [erin, frank, gina] = [await member("erin"), await member("frank"), await member("gina")]; + const res = await upload("group-icon", erin, await png(), "image/png", "group.png"); + const fileId = res.body.fileId as string; + + // Nothing points at it yet, so a picture chosen and then abandoned is swept. + assert.deepEqual(await unreferencedAmong([fileId]), [fileId]); + + const group = await createGroupConversation(erin.serverUserId, [frank.serverUserId, gina.serverUserId]); + await setConversationIcon(group.conversation_id, fileId); + assert.deepEqual(await unreferencedAmong([fileId]), []); + }); +}); diff --git a/src/routes/uploadAuthOrder.test.ts b/src/routes/uploadAuthOrder.test.ts index 986075cb..5cdd1b12 100644 --- a/src/routes/uploadAuthOrder.test.ts +++ b/src/routes/uploadAuthOrder.test.ts @@ -49,7 +49,7 @@ describe("multer never runs before authentication", () => { assert.deepEqual( buffering, - ["POST /api/server/icon", "POST /api/uploads/", "POST /api/uploads/avatar"], + ["POST /api/server/icon", "POST /api/uploads/", "POST /api/uploads/avatar", "POST /api/uploads/group-icon"], "the set of body-buffering routes changed; if that is deliberate, update this list", ); }); diff --git a/src/routes/uploadStorage.test.ts b/src/routes/uploadStorage.test.ts index ca63c1d0..ddce17d1 100644 --- a/src/routes/uploadStorage.test.ts +++ b/src/routes/uploadStorage.test.ts @@ -119,7 +119,8 @@ describe("the route uses it", () => { /** The attachment route only. The avatar route's own `insertFile` calls are unreachable with a sealed upload and would fail this for no reason. */ const routeStart = file.indexOf("uploadsRouter.post("); - const routeEnd = file.indexOf("uploadsRouter.post(", routeStart + 1); + // The route's own closing line: the avatar pipeline now sits between routes. + const routeEnd = file.indexOf("\n);\n", routeStart); const source = file.slice(routeStart, routeEnd); it("asks for the decision rather than working it out again", () => { diff --git a/src/routes/uploads.ts b/src/routes/uploads.ts index 3096b126..f2451cc6 100644 --- a/src/routes/uploads.ts +++ b/src/routes/uploads.ts @@ -275,24 +275,17 @@ uploadsRouter.post( }, ); -uploadsRouter.post( - "/avatar", - requireBearerToken, - /* `upload_avatar_image`, not `change_avatar`: this endpoint only ever gets a - picture, so there is no flag for a modified client to lie about. */ - (req: Request, res: Response, next: NextFunction): void => { - ensurePermission(req, res, "upload_avatar_image") - .then((ok) => { if (ok) next(); }) - .catch(next); - }, - uploadAvatarToMemory("file"), +/** One pipeline for both pictures. Only an avatar is put on the uploader's own + row; a group picture doing that was GRYT-1182. */ +const storeAvatarImage = (purpose: "avatar" | "group") => (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 = purpose === "avatar" ? "Avatar" : "Group picture"; const disableS3 = (process.env.DISABLE_S3 || "").toLowerCase() === "true"; - if (disableS3) { res.status(503).json({ error: "s3_disabled", message: "S3 is disabled (DISABLE_S3=true). Avatar upload is unavailable." }); return; } + if (disableS3) { res.status(503).json({ error: "s3_disabled", message: `S3 is disabled (DISABLE_S3=true). ${what} upload is unavailable.` }); return; } const bucket = process.env.S3_BUCKET as string; if (!bucket) { res.status(500).json({ error: "s3_not_configured", message: "S3_BUCKET not configured" }); return; } @@ -314,7 +307,7 @@ uploadsRouter.post( if (typeof maxBytes === "number" && maxBytes > 0 && file.size > maxBytes) { res.status(413).json({ error: "file_too_large", - message: `Avatar too large. Max ${(maxBytes / (1024 * 1024)).toFixed(1)}MB.`, + message: `${what} too large. Max ${(maxBytes / (1024 * 1024)).toFixed(1)}MB.`, }); return; } @@ -355,7 +348,7 @@ uploadsRouter.post( uploaded_by_server_user_id: serverUserId, }); - await setUserAvatar(serverUserId, fileId); + if (purpose === "avatar") await setUserAvatar(serverUserId, fileId); res.json({ fileId, processing: false }); return; } @@ -454,8 +447,8 @@ uploadsRouter.post( : /AccessDenied|Forbidden/i.test(raw) ? "File storage access denied. Please contact the server administrator." : raw.length > 0 - ? `Avatar upload failed: ${raw}` - : "Avatar upload failed due to a storage error."; + ? `${what} upload failed: ${raw}` + : `${what} upload failed due to a storage error.`; res.status(502).json({ error: "s3_error", message: friendly }); return; } @@ -479,8 +472,12 @@ uploadsRouter.post( created_at: new Date(), }); - await updateUserAvatar(serverUserId, fileId); - res.status(201).json({ avatarFileId: fileId, processing }); + if (purpose === "avatar") { + await updateUserAvatar(serverUserId, fileId); + res.status(201).json({ avatarFileId: fileId, processing }); + } else { + res.status(201).json({ fileId, processing }); + } // Background: resize oversized animated file and replace the placeholder if (processing) { @@ -526,7 +523,33 @@ uploadsRouter.post( } }) .catch(next); + }; + +uploadsRouter.post( + "/avatar", + requireBearerToken, + /* `upload_avatar_image`, not `change_avatar`: this endpoint only ever gets a + picture, so there is no flag for a modified client to lie about. */ + (req: Request, res: Response, next: NextFunction): void => { + ensurePermission(req, res, "upload_avatar_image") + .then((ok) => { if (ok) next(); }) + .catch(next); }, + uploadAvatarToMemory("file"), + storeAvatarImage("avatar"), +); + +uploadsRouter.post( + "/group-icon", + requireBearerToken, + // The permission the group handlers ask for, since only they use this file. + (req: Request, res: Response, next: NextFunction): void => { + ensurePermission(req, res, "send_direct_messages") + .then((ok) => { if (ok) next(); }) + .catch(next); + }, + uploadAvatarToMemory("file"), + storeAvatarImage("group"), ); uploadsRouter.delete(