Allow file access not in Attachments table (#5876)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

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",

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/<uuid>/<uuid>/<name> or public/<uuid>/<uuid>/<name>"
+  });
+
+  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/<uuid>/<uuid>/<name> or public/<uuid>/<uuid>/<name>"
-    );
   });
 
-  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");
+  });
 });

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<T.FilesGetReq>) => {
-    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<T.FilesGetReq>): 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;

server/models/Attachment.test.ts

@@ -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);
-  });
-});

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<Attachment> {
-    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<Attachment> {
-    return this.findOne({
-      where: { key },
-      rejectOnEmpty: true,
-    });
-  }
-
   // associations
 
   @BelongsTo(() => Team, "teamId")

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
    *

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<string>((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);
   }
 }

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"