From a3b2615edf4e3e3f11e5f9b392ad319ac42bf48e Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Wed, 24 Dec 2025 20:24:58 -0500 Subject: [PATCH] chore: Remove future public bucket usage (#10977) * No longer upload avatars to public bucket * Public redirect * tests * test * test --- .../tasks/UploadLinearWorkspaceLogoTask.ts | 33 ++++++++++------- plugins/storage/server/api/files.test.ts | 11 +++++- server/commands/attachmentCreator.ts | 1 - server/models/Attachment.ts | 12 +++++++ .../models/helpers/AttachmentHelper.test.ts | 18 ++-------- server/models/helpers/AttachmentHelper.ts | 6 +--- server/queues/tasks/APIImportTask.ts | 1 - server/queues/tasks/UploadTeamAvatarTask.ts | 34 ++++++++++++------ server/queues/tasks/UploadUserAvatarTask.ts | 23 ++++++------ .../api/attachments/attachments.test.ts | 35 ++++++++++++++++--- server/routes/api/attachments/attachments.ts | 25 +++++-------- server/routes/api/documents/documents.ts | 1 - server/test/factories.ts | 2 +- 13 files changed, 121 insertions(+), 81 deletions(-) diff --git a/plugins/linear/server/tasks/UploadLinearWorkspaceLogoTask.ts b/plugins/linear/server/tasks/UploadLinearWorkspaceLogoTask.ts index 7230bc3f8f..28d491ac0f 100644 --- a/plugins/linear/server/tasks/UploadLinearWorkspaceLogoTask.ts +++ b/plugins/linear/server/tasks/UploadLinearWorkspaceLogoTask.ts @@ -1,9 +1,9 @@ import type { IntegrationType } from "@shared/types"; -import { IntegrationService } from "@shared/types"; -import { Integration } from "@server/models"; -import { Buckets } from "@server/models/helpers/AttachmentHelper"; +import { IntegrationService, AttachmentPreset } from "@shared/types"; +import attachmentCreator from "@server/commands/attachmentCreator"; +import { createContext } from "@server/context"; +import { Integration, User } from "@server/models"; import { BaseTask, TaskPriority } from "@server/queues/tasks/base/BaseTask"; -import FileStorage from "@server/storage/files"; type Props = { /** The integrationId to operate on */ @@ -25,19 +25,26 @@ export default class UploadLinearWorkspaceLogoTask extends BaseTask { return; } - const res = await FileStorage.storeFromUrl( - props.logoUrl, - `${Buckets.avatars}/${integration.teamId}/${crypto.randomUUID()}`, - "public-read", - { + const user = await User.findByPk(integration.userId); + if (!user) { + return; + } + + const attachment = await attachmentCreator({ + name: "logo", + url: props.logoUrl, + user, + preset: AttachmentPreset.Avatar, + ctx: createContext({ user }), + fetchOptions: { headers: { Authorization: `Bearer ${integration.authentication.token}`, }, - } - ); + }, + }); - if (res?.url) { - integration.settings.linear!.workspace.logoUrl = res.url; + if (attachment) { + integration.settings.linear!.workspace.logoUrl = attachment.url; integration.changed("settings", true); await integration.save(); } diff --git a/plugins/storage/server/api/files.test.ts b/plugins/storage/server/api/files.test.ts index 118d2eff98..e8daed34c5 100644 --- a/plugins/storage/server/api/files.test.ts +++ b/plugins/storage/server/api/files.test.ts @@ -5,7 +5,9 @@ import FormData from "form-data"; import { ensureDirSync } from "fs-extra"; import { FileOperationState, FileOperationType } from "@shared/types"; import env from "@server/env"; -import { Buckets } from "@server/models/helpers/AttachmentHelper"; +import AttachmentHelper, { + Buckets, +} from "@server/models/helpers/AttachmentHelper"; import FileStorage from "@server/storage/files"; import { buildAttachment, @@ -13,6 +15,7 @@ import { buildUser, } from "@server/test/factories"; import { getTestServer } from "@server/test/support"; +import { randomUUID } from "crypto"; const server = getTestServer(); @@ -175,6 +178,12 @@ describe("#files.get", () => { const attachment = await buildAttachment( { + acl: "public-read", + key: AttachmentHelper.getKey({ + id: randomUUID(), + name: fileName, + userId: user.id, + }).replace(Buckets.uploads, Buckets.public), teamId: user.teamId, userId: user.id, contentType: diff --git a/server/commands/attachmentCreator.ts b/server/commands/attachmentCreator.ts index 4c33cc9d6e..78e1c908b9 100644 --- a/server/commands/attachmentCreator.ts +++ b/server/commands/attachmentCreator.ts @@ -47,7 +47,6 @@ export default async function attachmentCreator({ }: Props): Promise { const acl = AttachmentHelper.presetToAcl(preset); const key = AttachmentHelper.getKey({ - acl, id: randomUUID(), name, userId: user.id, diff --git a/server/models/Attachment.ts b/server/models/Attachment.ts index b3ca96bb57..7b682ee596 100644 --- a/server/models/Attachment.ts +++ b/server/models/Attachment.ts @@ -32,6 +32,7 @@ import { SkipChangeset } from "./decorators/Changeset"; import Fix from "./decorators/Fix"; import Length from "./validators/Length"; import Logger from "@server/logging/Logger"; +import { Buckets } from "./helpers/AttachmentHelper"; @Table({ tableName: "attachments", modelName: "attachment" }) @Fix @@ -78,6 +79,17 @@ class Attachment extends IdModel< return path.parse(this.key).base; } + /** + * Whether the attachment is stored in a public bucket. This does not relate + * to the ACL of the attachment itself. Previously "public" attachments were + * stored in a separate bucket – now all attachments are stored in a private + * bucket and ACL is checked per attachment. + */ + get isStoredInPublicBucket() { + const bucket = this.key.split("/")[0]; + return [Buckets.avatars, Buckets.public].includes(bucket as Buckets); + } + /** * Whether the attachment is private or not. */ diff --git a/server/models/helpers/AttachmentHelper.test.ts b/server/models/helpers/AttachmentHelper.test.ts index 1a4f6bc50d..958f45b45c 100644 --- a/server/models/helpers/AttachmentHelper.test.ts +++ b/server/models/helpers/AttachmentHelper.test.ts @@ -2,20 +2,8 @@ import AttachmentHelper from "./AttachmentHelper"; describe("AttachmentHelper", () => { describe("getKey", () => { - it("should return the correct key for a public attachment", () => { - const key = AttachmentHelper.getKey({ - acl: "public-read", - id: "123", - name: "test.png", - userId: "456", - }); - - expect(key).toEqual("public/456/123/test.png"); - }); - it("should return the correct key for a private attachment", () => { const key = AttachmentHelper.getKey({ - acl: "private", id: "123", name: "test.png", userId: "456", @@ -26,26 +14,24 @@ describe("AttachmentHelper", () => { it("should return the correct key for a long file name", () => { const key = AttachmentHelper.getKey({ - acl: "public-read", id: "123", name: "a".repeat(300), userId: "456", }); expect(key).toEqual( - `public/456/123/${"a".repeat(AttachmentHelper.maximumFileNameLength)}` + `uploads/456/123/${"a".repeat(AttachmentHelper.maximumFileNameLength)}` ); }); it("should remove invalid characters from the key", () => { const key = AttachmentHelper.getKey({ - acl: "public-read", id: "123", name: "test/../one.png", userId: "456", }); - expect(key).toEqual("public/456/123/test/one.png"); + expect(key).toEqual("uploads/456/123/test/one.png"); }); }); }); diff --git a/server/models/helpers/AttachmentHelper.ts b/server/models/helpers/AttachmentHelper.ts index 8979532b8a..ced41f6606 100644 --- a/server/models/helpers/AttachmentHelper.ts +++ b/server/models/helpers/AttachmentHelper.ts @@ -16,24 +16,20 @@ export default class AttachmentHelper { /** * Get the upload location for the given upload details * - * @param acl The ACL to use * @param id The ID of the attachment * @param name The name of the attachment * @param userId The ID of the user uploading the attachment */ static getKey({ - acl, id, name, userId, }: { - acl: string; id: string; name: string; userId: string; }) { - const bucket = acl === "public-read" ? Buckets.public : Buckets.uploads; - const keyPrefix = `${bucket}/${userId}/${id}`; + const keyPrefix = `${Buckets.uploads}/${userId}/${id}`; return ValidateKey.sanitize( `${keyPrefix}/${name.slice(0, this.maximumFileNameLength)}` ); diff --git a/server/queues/tasks/APIImportTask.ts b/server/queues/tasks/APIImportTask.ts index 6662bdeeda..19aa41c738 100644 --- a/server/queues/tasks/APIImportTask.ts +++ b/server/queues/tasks/APIImportTask.ts @@ -295,7 +295,6 @@ export default abstract class APIImportTask< AttachmentPreset.DocumentAttachment ); const key = AttachmentHelper.getKey({ - acl, id: modelId, name: item.name, userId: createdBy.id, diff --git a/server/queues/tasks/UploadTeamAvatarTask.ts b/server/queues/tasks/UploadTeamAvatarTask.ts index bbce066b8c..fbf99ea693 100644 --- a/server/queues/tasks/UploadTeamAvatarTask.ts +++ b/server/queues/tasks/UploadTeamAvatarTask.ts @@ -1,7 +1,7 @@ -import { randomUUID } from "crypto"; -import { Team } from "@server/models"; -import { Buckets } from "@server/models/helpers/AttachmentHelper"; -import FileStorage from "@server/storage/files"; +import { AttachmentPreset } from "@shared/types"; +import attachmentCreator from "@server/commands/attachmentCreator"; +import { createContext } from "@server/context"; +import { Team, User } from "@server/models"; import { BaseTask, TaskPriority } from "./base/BaseTask"; type Props = { @@ -21,14 +21,26 @@ export default class UploadTeamAvatarTask extends BaseTask { rejectOnEmpty: true, }); - const res = await FileStorage.storeFromUrl( - props.avatarUrl, - `${Buckets.avatars}/${team.id}/${randomUUID()}`, - "public-read" - ); + const user = await User.findOne({ + where: { + teamId: team.id, + }, + }); - if (res?.url) { - await team.update({ avatarUrl: res.url }); + if (!user) { + return; + } + + const attachment = await attachmentCreator({ + name: "avatar", + url: props.avatarUrl, + user, + preset: AttachmentPreset.Avatar, + ctx: createContext({ user }), + }); + + if (attachment) { + await team.update({ avatarUrl: attachment.url }); } } diff --git a/server/queues/tasks/UploadUserAvatarTask.ts b/server/queues/tasks/UploadUserAvatarTask.ts index 66a1360677..ec40b45f37 100644 --- a/server/queues/tasks/UploadUserAvatarTask.ts +++ b/server/queues/tasks/UploadUserAvatarTask.ts @@ -1,7 +1,8 @@ -import { createHash, randomUUID } from "crypto"; +import { createHash } from "crypto"; +import { AttachmentPreset } from "@shared/types"; +import attachmentCreator from "@server/commands/attachmentCreator"; +import { createContext } from "@server/context"; import { User } from "@server/models"; -import { Buckets } from "@server/models/helpers/AttachmentHelper"; -import FileStorage from "@server/storage/files"; import { BaseTask, TaskPriority } from "./base/BaseTask"; type Props = { @@ -28,14 +29,16 @@ export default class UploadUserAvatarTask extends BaseTask { return; } - const res = await FileStorage.storeFromUrl( - props.avatarUrl, - `${Buckets.avatars}/${user.id}/${randomUUID()}/${hash}`, - "public-read" - ); + const attachment = await attachmentCreator({ + name: hash, + url: props.avatarUrl, + user, + preset: AttachmentPreset.Avatar, + ctx: createContext({ user }), + }); - if (res?.url) { - await user.update({ avatarUrl: res.url }); + if (attachment) { + await user.update({ avatarUrl: attachment.url }); } } diff --git a/server/routes/api/attachments/attachments.test.ts b/server/routes/api/attachments/attachments.test.ts index d60978ba00..585020361a 100644 --- a/server/routes/api/attachments/attachments.test.ts +++ b/server/routes/api/attachments/attachments.test.ts @@ -1,3 +1,4 @@ +import { randomUUID } from "crypto"; import { AttachmentPreset, CollectionPermission } from "@shared/types"; import { UserMembership } from "@server/models"; import Attachment from "@server/models/Attachment"; @@ -425,11 +426,6 @@ describe("#attachments.delete", () => { }); describe("#attachments.redirect", () => { - it("should require authentication", async () => { - const res = await server.post("/api/attachments.redirect"); - expect(res.status).toEqual(401); - }); - it("should return a redirect for an attachment belonging to a document user has access to", async () => { const user = await buildUser(); const attachment = await buildAttachment({ @@ -518,6 +514,35 @@ describe("#attachments.redirect", () => { expect(res.status).toEqual(302); }); + it("should return a redirect for an attachment in a public bucket without authentication", async () => { + const attachment = await buildAttachment({ + key: `public/${randomUUID()}/test.png`, + acl: "public-read", + }); + const res = await server.post("/api/attachments.redirect", { + body: { + id: attachment.id, + }, + redirect: "manual", + }); + expect(res.status).toEqual(302); + expect(res.headers.get("location")).toContain(attachment.canonicalUrl); + }); + + it("should return a redirect for a public-read attachment without authentication (not in public bucket)", async () => { + const attachment = await buildAttachment({ + acl: "public-read", + }); + const res = await server.post("/api/attachments.redirect", { + body: { + id: attachment.id, + }, + redirect: "manual", + }); + expect(res.status).toEqual(302); + expect(res.headers.get("location")).toContain(await attachment.signedUrl); + }); + it("should not return a redirect for a private attachment belonging to a document user does not have access to", async () => { const user = await buildUser(); const collection = await buildCollection({ diff --git a/server/routes/api/attachments/attachments.ts b/server/routes/api/attachments/attachments.ts index fe90f3cda6..6a96ebf8b4 100644 --- a/server/routes/api/attachments/attachments.ts +++ b/server/routes/api/attachments/attachments.ts @@ -121,7 +121,6 @@ router.post( const modelId = id ?? randomUUID(); const acl = AttachmentHelper.presetToAcl(preset); const key = AttachmentHelper.getKey({ - acl, id: modelId, name, userId: user.id, @@ -157,12 +156,7 @@ router.post( }, attachment: { ...presentAttachment(attachment), - // always use the redirect url for document attachments, as the serializer - // depends on it to detect attachment vs link - url: - preset === AttachmentPreset.DocumentAttachment - ? attachment.redirectUrl - : attachment.url, + url: attachment.redirectUrl, }, }, }; @@ -193,7 +187,6 @@ router.post( const modelId = id ?? randomUUID(); const acl = AttachmentHelper.presetToAcl(preset); const key = AttachmentHelper.getKey({ - acl, id: modelId, name, userId: user.id, @@ -278,12 +271,12 @@ const handleAttachmentsRedirect = async ( ) => { const id = (ctx.input.body.id ?? ctx.input.query.id) as string; - const { user } = ctx.state.auth; + const user = ctx.state.auth?.user; const attachment = await Attachment.findByPk(id, { rejectOnEmpty: true, }); - if (attachment.isPrivate && attachment.teamId !== user.teamId) { + if (attachment.isPrivate && attachment.teamId !== user?.teamId) { throw AuthorizationError(); } @@ -296,27 +289,27 @@ const handleAttachmentsRedirect = async ( } ); - if (attachment.isPrivate) { + if (attachment.isStoredInPublicBucket) { + ctx.set("Cache-Control", `max-age=604800, immutable`); + ctx.redirect(attachment.canonicalUrl); + } else { ctx.set( "Cache-Control", `max-age=${BaseStorage.defaultSignedUrlExpires}, immutable` ); ctx.redirect(await attachment.signedUrl); - } else { - ctx.set("Cache-Control", `max-age=604800, immutable`); - ctx.redirect(attachment.canonicalUrl); } }; router.get( "attachments.redirect", - auth(), + auth({ optional: true }), validate(T.AttachmentsRedirectSchema), handleAttachmentsRedirect ); router.post( "attachments.redirect", - auth(), + auth({ optional: true }), validate(T.AttachmentsRedirectSchema), handleAttachmentsRedirect ); diff --git a/server/routes/api/documents/documents.ts b/server/routes/api/documents/documents.ts index e8236677fd..4f3bf8911a 100644 --- a/server/routes/api/documents/documents.ts +++ b/server/routes/api/documents/documents.ts @@ -1514,7 +1514,6 @@ router.post( const acl = "private"; const key = AttachmentHelper.getKey({ - acl, id: randomUUID(), name: fileName, userId: user.id, diff --git a/server/test/factories.ts b/server/test/factories.ts index 77a6e0589b..8c967c151d 100644 --- a/server/test/factories.ts +++ b/server/test/factories.ts @@ -564,7 +564,7 @@ export async function buildAttachment( const name = fileName || faker.system.fileName(); return Attachment.create({ id, - key: AttachmentHelper.getKey({ acl, id, name, userId: overrides.userId }), + key: AttachmentHelper.getKey({ id, name, userId: overrides.userId }), contentType: "image/png", size: 100, acl,