fix: files.create permissions (#5877)

* fix: files.create permissions

* test

* new

plugins/storage/server/api/files.test.ts

@@ -24,30 +24,68 @@ describe("#files.create", () => {
     expect(res.status).toEqual(400);
   });
 
-  it("should fail with status 404 if existing file is requested with key", async () => {
+  it("should fail with status 401 if associated attachment does not belong to user", async () => {
     const user = await buildUser();
     const fileName = "images.docx";
-    const key = path.join("uploads", user.id, uuidV4(), fileName);
+    const attachment = await buildAttachment(
+      {
+        teamId: user.teamId,
+        contentType:
+          "application/vnd.openxmlformats-officedocument.wordprocessingml.document",
+      },
+      fileName
+    );
+
+    const content = await readFile(
+      path.resolve(__dirname, "..", "test", "fixtures", fileName)
+    );
+    const form = new FormData();
+    form.append("key", attachment.key);
+    form.append("file", content, fileName);
+    form.append("token", user.getJwtToken());
+
+    const res = await server.post(`/api/files.create`, {
+      headers: form.getHeaders(),
+      body: form,
+    });
+    expect(res.status).toEqual(403);
+  });
+
+  it("should fail with status 401 if file exists on disk", async () => {
+    const user = await buildUser();
+    const fileName = "images.docx";
+    const attachment = await buildAttachment(
+      {
+        userId: user.id,
+        teamId: user.teamId,
+        contentType:
+          "application/vnd.openxmlformats-officedocument.wordprocessingml.document",
+      },
+      fileName
+    );
 
     ensureDirSync(
-      path.dirname(path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key))
+      path.dirname(path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, attachment.key))
     );
 
     copyFileSync(
       path.resolve(__dirname, "..", "test", "fixtures", fileName),
-      path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, key)
+      path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, attachment.key)
     );
 
-    const res = await server.get(`/api/files.get?key=${key}`);
-    expect(res.status).toEqual(404);
-  });
+    const content = await readFile(
+      path.resolve(__dirname, "..", "test", "fixtures", fileName)
+    );
+    const form = new FormData();
+    form.append("key", attachment.key);
+    form.append("file", content, fileName);
+    form.append("token", user.getJwtToken());
 
-  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);
+    const res = await server.post(`/api/files.create`, {
+      headers: form.getHeaders(),
+      body: form,
+    });
+    expect(res.status).toEqual(400);
   });
 
   it("should succeed with status 200 ok and create a file", async () => {
@@ -86,6 +124,32 @@ describe("#files.create", () => {
 });
 
 describe("#files.get", () => {
+  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 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`);
     expect(res.status).toEqual(400);

plugins/storage/server/api/files.ts

@@ -2,7 +2,11 @@ import JWT from "jsonwebtoken";
 import Router from "koa-router";
 import mime from "mime-types";
 import env from "@server/env";
-import { AuthenticationError, ValidationError } from "@server/errors";
+import {
+  AuthenticationError,
+  AuthorizationError,
+  ValidationError,
+} from "@server/errors";
 import auth from "@server/middlewares/authentication";
 import multipart from "@server/middlewares/multipart";
 import { rateLimiter } from "@server/middlewares/rateLimiter";
@@ -37,11 +41,11 @@ router.post(
       rejectOnEmpty: true,
     });
 
-    if (attachment.isPrivate) {
-      authorize(actor, "createAttachment", actor.team);
+    if (attachment?.userId !== actor.id) {
+      throw AuthorizationError("Invalid key");
     }
 
-    await attachment.overwriteFile(file);
+    await attachment.writeFile(file);
 
     ctx.body = {
       success: true,

server/models/Attachment.ts

@@ -116,12 +116,12 @@ class Attachment extends IdModel {
 
   /**
    * Store the given file in storage at the location specified by the attachment key.
-   * If the attachment already exists, it will be overwritten.
+   * If the attachment already exists, an error will be thrown.
    *
    * @param file The file to store
    * @returns A promise resolving to the attachment
    */
-  async overwriteFile(file: File) {
+  async writeFile(file: File) {
     return FileStorage.store({
       body: createReadStream(file.filepath),
       contentLength: file.size,

server/storage/files/LocalStorage.ts

@@ -1,19 +1,20 @@
 import { Blob } from "buffer";
-import {
-  ReadStream,
-  closeSync,
-  createReadStream,
-  createWriteStream,
-  existsSync,
-  openSync,
-} from "fs";
 import { mkdir, unlink } from "fs/promises";
 import path from "path";
 import { Readable } from "stream";
+import {
+  ReadStream,
+  close,
+  pathExists,
+  createReadStream,
+  createWriteStream,
+  open,
+} from "fs-extra";
 import invariant from "invariant";
 import JWT from "jsonwebtoken";
 import safeResolvePath from "resolve-path";
 import env from "@server/env";
+import { ValidationError } from "@server/errors";
 import Logger from "@server/logging/Logger";
 import BaseStorage from "./BaseStorage";
 
@@ -53,12 +54,14 @@ export default class LocalStorage extends BaseStorage {
     key: string;
     acl?: string;
   }) => {
-    const subdir = key.split("/").slice(0, -1).join("/");
-    if (!existsSync(path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, subdir))) {
-      await mkdir(path.join(env.FILE_STORAGE_LOCAL_ROOT_DIR, subdir), {
+    const exists = await pathExists(this.getFilePath(key));
+    if (exists) {
+      throw ValidationError(`File already exists at ${key}`);
+    }
+
+    await mkdir(this.getFilePath(path.dirname(key)), {
       recursive: true,
     });
-    }
 
     let src: NodeJS.ReadableStream;
     if (body instanceof ReadStream) {
@@ -70,7 +73,9 @@ export default class LocalStorage extends BaseStorage {
     }
 
     const filePath = this.getFilePath(key);
-    closeSync(openSync(filePath, "w"));
+
+    // Create the file on disk first
+    await open(filePath, "w").then(close);
 
     return new Promise<string>((resolve, reject) => {
       const dest = createWriteStream(filePath)