diff --git a/package.json b/package.json index 8142b992e..8b3c7a6f1 100644 --- a/package.json +++ b/package.json @@ -198,6 +198,7 @@ "reflect-metadata": "^0.1.13", "refractor": "^3.6.0", "request-filtering-agent": "^1.1.2", + "resolve-path": "^1.4.0", "rfc6902": "^5.0.1", "sanitize-filename": "^1.6.3", "semver": "^7.5.2", @@ -291,6 +292,7 @@ "@types/readable-stream": "^4.0.2", "@types/redis-info": "^3.0.0", "@types/refractor": "^3.0.2", + "@types/resolve-path": "^1.4.0", "@types/semver": "^7.5.2", "@types/sequelize": "^4.28.10", "@types/slug": "^5.0.3", diff --git a/plugins/storage/server/api/files.test.ts b/plugins/storage/server/api/files.test.ts index 6683061d0..de7b5c877 100644 --- a/plugins/storage/server/api/files.test.ts +++ b/plugins/storage/server/api/files.test.ts @@ -1,9 +1,12 @@ -import { existsSync } from "fs"; +import { existsSync, copyFileSync } from "fs"; import { readFile } from "fs/promises"; import path from "path"; import FormData from "form-data"; +import { ensureDirSync } from "fs-extra"; +import { v4 as uuidV4 } from "uuid"; import env from "@server/env"; import "@server/test/env"; +import FileStorage from "@server/storage/files"; import { buildAttachment, buildUser } from "@server/test/factories"; import { getTestServer } from "@server/test/support"; @@ -18,11 +21,33 @@ describe("#files.create", () => { key: "public/foo/bar/baz.png", }, }); - const body = await res.json(); expect(res.status).toEqual(400); - expect(body.message).toEqual( - "key: Must be of the form uploads/// or public///" + }); + + it("should fail with status 404 if existing file is requested with key", async () => { + const user = await buildUser(); + const fileName = "images.docx"; + const key = path.join("uploads", user.id, uuidV4(), fileName); + + ensureDirSync( + path.dirname(path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key)) ); + + copyFileSync( + path.resolve(__dirname, "..", "test", "fixtures", fileName), + path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key) + ); + + const res = await server.get(`/api/files.get?key=${key}`); + expect(res.status).toEqual(404); + }); + + it("should fail with status 404 if non-existing file is requested with key", async () => { + const user = await buildUser(); + const fileName = "images.docx"; + const key = path.join("uploads", user.id, uuidV4(), fileName); + const res = await server.get(`/api/files.get?key=${key}`); + expect(res.status).toEqual(404); }); it("should succeed with status 200 ok and create a file", async () => { @@ -63,21 +88,17 @@ describe("#files.create", () => { describe("#files.get", () => { it("should fail with status 400 bad request if key is invalid", async () => { const res = await server.get(`/api/files.get?key=public/foo/bar/baz.png`); - const body = await res.json(); expect(res.status).toEqual(400); - expect(body.message).toEqual( - "key: Must be of the form uploads/// or public///" - ); }); - it("should fail with status 400 bad request if none of key or sig is supplied", async () => { + it("should fail with status 400 bad request if neither key or sig is supplied", async () => { const res = await server.get("/api/files.get"); const body = await res.json(); expect(res.status).toEqual(400); expect(body.message).toEqual("query: One of key or sig is required"); }); - it("should succeed with status 200 ok when file is requested using key", async () => { + it("should succeed with status 200 ok when attachment is requested using key", async () => { const user = await buildUser(); const fileName = "images.docx"; @@ -112,7 +133,7 @@ describe("#files.get", () => { ); }); - it("should succeed with status 200 ok when private file is requested using signature", async () => { + it("should succeed with status 200 ok when private attachment is requested using signature", async () => { const user = await buildUser(); const fileName = "images.docx"; @@ -147,4 +168,48 @@ describe("#files.get", () => { 'attachment; filename="images.docx"' ); }); + + it("should succeed with status 200 ok when file is requested using signature", async () => { + const user = await buildUser(); + const fileName = "images.docx"; + const key = path.join("uploads", user.id, uuidV4(), fileName); + const signedUrl = await FileStorage.getSignedUrl(key); + + ensureDirSync( + path.dirname(path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key)) + ); + + copyFileSync( + path.resolve(__dirname, "..", "test", "fixtures", fileName), + path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key) + ); + + const res = await server.get(signedUrl); + expect(res.status).toEqual(200); + expect(res.headers.get("Content-Type")).toEqual( + "application/vnd.openxmlformats-officedocument.wordprocessingml.document" + ); + expect(res.headers.get("Content-Disposition")).toEqual( + 'attachment; filename="images.docx"' + ); + }); + + it("should succeed with status 200 ok when avatar is requested using key", async () => { + const user = await buildUser(); + const key = path.join("avatars", user.id, uuidV4()); + + ensureDirSync( + path.dirname(path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key)) + ); + + copyFileSync( + path.resolve(__dirname, "..", "test", "fixtures", "avatar.jpg"), + path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key) + ); + + const res = await server.get(`/api/files.get?key=${key}`); + expect(res.status).toEqual(200); + expect(res.headers.get("Content-Type")).toEqual("application/octet-stream"); + expect(res.headers.get("Content-Disposition")).toEqual("attachment"); + }); }); diff --git a/plugins/storage/server/api/files.ts b/plugins/storage/server/api/files.ts index ef39a4ff1..de59497e3 100644 --- a/plugins/storage/server/api/files.ts +++ b/plugins/storage/server/api/files.ts @@ -1,14 +1,19 @@ +import JWT from "jsonwebtoken"; import Router from "koa-router"; +import mime from "mime-types"; import env from "@server/env"; -import { ValidationError } from "@server/errors"; +import { AuthenticationError, ValidationError } from "@server/errors"; import auth from "@server/middlewares/authentication"; import multipart from "@server/middlewares/multipart"; import { rateLimiter } from "@server/middlewares/rateLimiter"; import validate from "@server/middlewares/validate"; import { Attachment } from "@server/models"; +import AttachmentHelper from "@server/models/helpers/AttachmentHelper"; import { authorize } from "@server/policies"; +import FileStorage from "@server/storage/files"; import { APIContext } from "@server/types"; import { RateLimiterStrategy } from "@server/utils/RateLimiter"; +import { getJWTPayload } from "@server/utils/jwt"; import { createRootDirForLocalStorage } from "../utils"; import * as T from "./schema"; @@ -27,7 +32,10 @@ router.post( const { key } = ctx.input.body; const file = ctx.input.file; - const attachment = await Attachment.findByKey(key); + const attachment = await Attachment.findOne({ + where: { key }, + rejectOnEmpty: true, + }); if (attachment.isPrivate) { authorize(actor, "createAttachment", actor.team); @@ -46,26 +54,60 @@ router.get( auth({ optional: true }), validate(T.FilesGetSchema), async (ctx: APIContext) => { - const { key, sig } = ctx.input.query; const actor = ctx.state.auth.user; - let attachment: Attachment | null; + const key = getKeyFromContext(ctx); + const isSignedRequest = !!ctx.input.query.sig; + const { isPublicBucket, fileName } = AttachmentHelper.parseKey(key); + const skipAuthorize = isPublicBucket || isSignedRequest; + const cacheHeader = "max-age=604800, immutable"; - if (key) { - attachment = await Attachment.findByKey(key); - - if (attachment.isPrivate) { - authorize(actor, "read", attachment); - } - } else if (sig) { - attachment = await Attachment.findBySignature(sig); + if (skipAuthorize) { + ctx.set("Cache-Control", cacheHeader); + ctx.set( + "Content-Type", + (fileName ? mime.lookup(fileName) : undefined) || + "application/octet-stream" + ); + ctx.attachment(fileName); + ctx.body = FileStorage.getFileStream(key); } else { - throw ValidationError("Must provide either key or signature"); - } + const attachment = await Attachment.findOne({ + where: { key }, + rejectOnEmpty: true, + }); + authorize(actor, "read", attachment); - ctx.set("Content-Type", attachment.contentType); - ctx.attachment(attachment.name); - ctx.body = attachment.stream; + ctx.set("Cache-Control", cacheHeader); + ctx.set("Content-Type", attachment.contentType); + ctx.attachment(attachment.name); + ctx.body = attachment.stream; + } } ); +function getKeyFromContext(ctx: APIContext): string { + const { key, sig } = ctx.input.query; + if (sig) { + const payload = getJWTPayload(sig); + + if (payload.type !== "attachment") { + throw AuthenticationError("Invalid signature"); + } + + try { + JWT.verify(sig, env.SECRET_KEY); + } catch (err) { + throw AuthenticationError("Invalid signature"); + } + + return payload.key as string; + } + + if (key) { + return key; + } + + throw ValidationError("Must provide either key or sig parameter"); +} + export default router; diff --git a/plugins/storage/server/test/fixtures/avatar.jpg b/plugins/storage/server/test/fixtures/avatar.jpg new file mode 100644 index 000000000..856a55191 Binary files /dev/null and b/plugins/storage/server/test/fixtures/avatar.jpg differ diff --git a/server/models/Attachment.test.ts b/server/models/Attachment.test.ts deleted file mode 100644 index b220e705f..000000000 --- a/server/models/Attachment.test.ts +++ /dev/null @@ -1,10 +0,0 @@ -import { buildAttachment } from "@server/test/factories"; -import Attachment from "./Attachment"; - -describe("#findByKey", () => { - it("should return the correct attachment given a key", async () => { - const attachment = await buildAttachment(); - const found = await Attachment.findByKey(attachment.key); - expect(found?.id).toBe(attachment.id); - }); -}); diff --git a/server/models/Attachment.ts b/server/models/Attachment.ts index 49f1c2a2b..25e37eb08 100644 --- a/server/models/Attachment.ts +++ b/server/models/Attachment.ts @@ -1,7 +1,6 @@ import { createReadStream } from "fs"; import path from "path"; import { File } from "formidable"; -import JWT from "jsonwebtoken"; import { QueryTypes } from "sequelize"; import { BeforeDestroy, @@ -15,10 +14,7 @@ import { IsNumeric, BeforeUpdate, } from "sequelize-typescript"; -import env from "@server/env"; -import { AuthenticationError } from "@server/errors"; import FileStorage from "@server/storage/files"; -import { getJWTPayload } from "@server/utils/jwt"; import { ValidateKey } from "@server/validation"; import Document from "./Document"; import Team from "./Team"; @@ -172,42 +168,6 @@ class Attachment extends IdModel { return parseInt(result?.[0]?.total ?? "0", 10); } - /** - * Find an attachment given a JWT signature. - * - * @param sign - The signature that uniquely identifies an attachment - * @returns A promise resolving to attachment corresponding to the signature - * @throws {AuthenticationError} Invalid signature if the signature verification fails - */ - static async findBySignature(sign: string): Promise { - const payload = getJWTPayload(sign); - - if (payload.type !== "attachment") { - throw AuthenticationError("Invalid signature"); - } - - try { - JWT.verify(sign, env.SECRET_KEY); - } catch (err) { - throw AuthenticationError("Invalid signature"); - } - - return this.findByKey(payload.key); - } - - /** - * Find an attachment given a key - * - * @param key The key representing attachment file path - * @returns A promise resolving to attachment corresponding to the key - */ - static async findByKey(key: string): Promise { - return this.findOne({ - where: { key }, - rejectOnEmpty: true, - }); - } - // associations @BelongsTo(() => Team, "teamId") diff --git a/server/models/helpers/AttachmentHelper.ts b/server/models/helpers/AttachmentHelper.ts index 1f69e57a2..bad0890d0 100644 --- a/server/models/helpers/AttachmentHelper.ts +++ b/server/models/helpers/AttachmentHelper.ts @@ -33,6 +33,34 @@ export default class AttachmentHelper { return `${keyPrefix}/${name}`; } + /** + * Parse a key into its constituent parts + * + * @param key The key to parse + * @returns The constituent parts + */ + static parseKey(key: string): { + bucket: string; + userId: string; + id: string; + fileName: string | undefined; + isPublicBucket: boolean; + } { + const parts = key.split("/"); + const bucket = parts[0]; + const userId = parts[1]; + const id = parts[2]; + const [fileName] = parts.length > 3 ? parts.slice(-1) : []; + + return { + bucket, + userId, + id, + fileName, + isPublicBucket: bucket === Buckets.avatars || bucket === Buckets.public, + }; + } + /** * Get the ACL to use for a given attachment preset * diff --git a/server/storage/files/LocalStorage.ts b/server/storage/files/LocalStorage.ts index 93a5e8ac9..92edca3d6 100644 --- a/server/storage/files/LocalStorage.ts +++ b/server/storage/files/LocalStorage.ts @@ -12,6 +12,7 @@ import path from "path"; import { Readable } from "stream"; import invariant from "invariant"; import JWT from "jsonwebtoken"; +import safeResolvePath from "resolve-path"; import env from "@server/env"; import Logger from "@server/logging/Logger"; import BaseStorage from "./BaseStorage"; @@ -68,11 +69,11 @@ export default class LocalStorage extends BaseStorage { src = Readable.from(body); } - const destPath = path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key); - closeSync(openSync(destPath, "w")); + const filePath = this.getFilePath(key); + closeSync(openSync(filePath, "w")); return new Promise((resolve, reject) => { - const dest = createWriteStream(destPath) + const dest = createWriteStream(filePath) .on("error", reject) .on("finish", () => resolve(this.getUrlForKey(key))); @@ -86,7 +87,7 @@ export default class LocalStorage extends BaseStorage { }; public async deleteFile(key: string) { - const filePath = path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key); + const filePath = this.getFilePath(key); try { await unlink(filePath); } catch (err) { @@ -112,12 +113,15 @@ export default class LocalStorage extends BaseStorage { }; public getFileStream(key: string) { + return createReadStream(this.getFilePath(key)); + } + + private getFilePath(key: string) { invariant( env.FILE_STORAGE_LOCAL_ROOT_DIR, "FILE_STORAGE_LOCAL_ROOT_DIR is required" ); - const filePath = path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key); - return createReadStream(filePath); + return safeResolvePath(env.FILE_STORAGE_LOCAL_ROOT_DIR, key); } } diff --git a/yarn.lock b/yarn.lock index 589259c78..87b4f3673 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3436,6 +3436,11 @@ dependencies: "@types/prismjs" "*" +"@types/resolve-path@^1.4.0": + version "1.4.0" + resolved "https://registry.yarnpkg.com/@types/resolve-path/-/resolve-path-1.4.0.tgz#7e5abe36da0ba037e9d84348bef9e85db752cae4" + integrity sha512-Zh5blp0SEy8yHxnN3yOnInh9NtZHze1c6sl/Neg2or/MbEEoxg69qZScrSqe4BYIRQD722r/ouV822Av1mKwRA== + "@types/resolve@1.17.1": version "1.17.1" resolved "https://registry.yarnpkg.com/@types/resolve/-/resolve-1.17.1.tgz#3afd6ad8967c77e4376c598a82ddd58f46ec45d6" @@ -11605,7 +11610,7 @@ resolve-options@^1.1.0: resolve-path@^1.4.0: version "1.4.0" resolved "https://registry.yarnpkg.com/resolve-path/-/resolve-path-1.4.0.tgz#c4bda9f5efb2fce65247873ab36bb4d834fe16f7" - integrity sha1-xL2p9e+y/OZSR4c6s2u02DT+Fvc= + integrity sha512-i1xevIst/Qa+nA9olDxLWnLk8YZbi8R/7JPbCMcgyWaFR6bKWaexgJgEB5oc2PKMjYdrHynyz0NY+if+H98t1w== dependencies: http-errors "~1.6.2" path-is-absolute "1.0.1"