fix: Re-check file operation permissions at download (#13148)

* fix: Re-check file operation permissions at download

* feedback

* Address feedback and copy across to other findByPk overrides

server/models/Collection.test.ts

@@ -487,6 +487,30 @@ describe("#findByPk", () => {
     expect(response!.id).toBe(collection.id);
   });
 
+  it("should not allow a passed where to override the id", async () => {
+    const collection = await buildCollection();
+    const other = await buildCollection();
+
+    const response = await Collection.findByPk(collection.id, {
+      where: { id: other.id },
+    });
+    expect(response!.id).toBe(collection.id);
+
+    const byUrlId = await Collection.findByPk(collection.urlId, {
+      where: { urlId: other.urlId },
+    });
+    expect(byUrlId!.id).toBe(collection.id);
+  });
+
+  it("should throw the passed error when rejectOnEmpty is an error", async () => {
+    const error = new Error("does not exist");
+    await expect(
+      Collection.findByPk("0e8280ea-7b4c-40e5-98ba-ec8a2f00f5e8", {
+        rejectOnEmpty: error,
+      })
+    ).rejects.toThrow(error);
+  });
+
   it("should not return documentStructure by default", async () => {
     const collection = await buildCollection();
     const response = await Collection.findByPk(collection.id);

server/models/Collection.ts

@@ -682,15 +682,17 @@ class Collection extends ParanoidModel<
 
     if (isUUID(id)) {
       const collection = await scope.findOne({
+        ...rest,
         where: {
           id,
         },
-        ...rest,
         rejectOnEmpty: false,
       });
 
       if (!collection && rest.rejectOnEmpty) {
-        throw new EmptyResultError(`Collection doesn't exist with id: ${id}`);
+        throw rest.rejectOnEmpty instanceof Error
+          ? rest.rejectOnEmpty
+          : new EmptyResultError(`Collection doesn't exist with id: ${id}`);
       }
 
       return collection;
@@ -699,15 +701,17 @@ class Collection extends ParanoidModel<
     const match = id.match(UrlHelper.SLUG_URL_REGEX);
     if (match) {
       const collection = await scope.findOne({
+        ...rest,
         where: {
           urlId: match[1],
         },
-        ...rest,
         rejectOnEmpty: false,
       });
 
       if (!collection && rest.rejectOnEmpty) {
-        throw new EmptyResultError(`Collection doesn't exist with id: ${id}`);
+        throw rest.rejectOnEmpty instanceof Error
+          ? rest.rejectOnEmpty
+          : new EmptyResultError(`Collection doesn't exist with id: ${id}`);
       }
 
       return collection;

server/models/Document.test.ts

@@ -300,6 +300,30 @@ describe("#findByPk", () => {
     ).rejects.toThrow(EmptyResultError);
   });
 
+  it("should not allow a passed where to override the id", async () => {
+    const document = await buildDocument();
+    const other = await buildDocument();
+
+    const response = await Document.findByPk(document.id, {
+      where: { id: other.id },
+    });
+    expect(response?.id).toBe(document.id);
+
+    const byUrlId = await Document.findByPk(document.urlId, {
+      where: { urlId: other.urlId },
+    });
+    expect(byUrlId?.id).toBe(document.id);
+  });
+
+  it("should throw the passed error when rejectOnEmpty is an error", async () => {
+    const error = new Error("does not exist");
+    await expect(
+      Document.findByPk("0e8280ea-7b4c-40e5-98ba-ec8a2f00f5e8", {
+        rejectOnEmpty: error,
+      })
+    ).rejects.toThrow(error);
+  });
+
   it("should load state as a fallback when content is empty", async () => {
     const state = Buffer.from([1, 2, 3]);
     const document = await buildDocument();

server/models/Document.ts

@@ -820,15 +820,17 @@ class Document extends ArchivableModel<
 
     if (isUUID(id)) {
       const document = await scope.findOne({
+        ...rest,
         where: {
           id,
         },
-        ...rest,
         rejectOnEmpty: false,
       });
 
       if (!document && rest.rejectOnEmpty) {
-        throw new EmptyResultError(`Document doesn't exist with id: ${id}`);
+        throw rest.rejectOnEmpty instanceof Error
+          ? rest.rejectOnEmpty
+          : new EmptyResultError(`Document doesn't exist with id: ${id}`);
       }
 
       return document;
@@ -837,15 +839,17 @@ class Document extends ArchivableModel<
     const match = id.match(UrlHelper.SLUG_URL_REGEX);
     if (match) {
       const document = await scope.findOne({
+        ...rest,
         where: {
           urlId: match[1],
         },
-        ...rest,
         rejectOnEmpty: false,
       });
 
       if (!document && rest.rejectOnEmpty) {
-        throw new EmptyResultError(`Document doesn't exist with id: ${id}`);
+        throw rest.rejectOnEmpty instanceof Error
+          ? rest.rejectOnEmpty
+          : new EmptyResultError(`Document doesn't exist with id: ${id}`);
       }
 
       return document;

server/models/FileOperation.test.ts

@@ -0,0 +1,39 @@
+import {
+  buildFileOperation,
+  buildTeam,
+  buildUser,
+} from "@server/test/factories";
+import FileOperation from "./FileOperation";
+
+describe("FileOperation", () => {
+  describe("findByPk", () => {
+    it("should not allow a passed where to override the id", async () => {
+      const team = await buildTeam();
+      const user = await buildUser({ teamId: team.id });
+      const fileOperation = await buildFileOperation({
+        teamId: team.id,
+        userId: user.id,
+      });
+      const other = await buildFileOperation({
+        teamId: team.id,
+        userId: user.id,
+      });
+
+      const found = await FileOperation.findByPk(fileOperation.id, {
+        where: { id: other.id },
+      });
+
+      expect(found?.id).toEqual(fileOperation.id);
+    });
+
+    it("should throw the passed error when rejectOnEmpty is an error", async () => {
+      const error = new Error("does not exist");
+
+      await expect(
+        FileOperation.findByPk("3a1b2c3d-0000-4000-8000-000000000000", {
+          rejectOnEmpty: error,
+        })
+      ).rejects.toThrow(error);
+    });
+  });
+});

server/models/FileOperation.ts

@@ -1,12 +1,16 @@
 import type {
+  FindOptions,
+  Identifier,
   InferAttributes,
   InferCreationAttributes,
+  NonNullFindOptions,
   WhereOptions,
 } from "sequelize";
-import { Op } from "sequelize";
+import { EmptyResultError, Op } from "sequelize";
 import {
   ForeignKey,
   DefaultScope,
+  Scopes,
   Column,
   BeforeDestroy,
   BelongsTo,
@@ -30,6 +34,11 @@ export type FileOperationOptions = {
   permission?: CollectionPermission | null;
 };
 
+type AdditionalFindOptions = {
+  userId?: string;
+  rejectOnEmpty?: boolean | Error;
+};
+
 @DefaultScope(() => ({
   include: [
     {
@@ -52,6 +61,47 @@ export type FileOperationOptions = {
     },
   ],
 }))
+@Scopes(() => ({
+  withSource: (userId: string) => {
+    if (!userId) {
+      return {};
+    }
+
+    return {
+      include: [
+        {
+          model: User,
+          as: "user",
+          paranoid: false,
+        },
+        {
+          model: Collection.scope([
+            "defaultScope",
+            {
+              method: ["withMembership", userId],
+            },
+          ]),
+          as: "collection",
+          required: false,
+          paranoid: false,
+        },
+        {
+          model: Document.scope([
+            "defaultScope",
+            {
+              method: ["withMembership", userId, false],
+            },
+          ]),
+          as: "document",
+          // Content columns are not needed to authorize or present an export.
+          attributes: { exclude: ["text", "content", "state"] },
+          required: false,
+          paranoid: false,
+        },
+      ],
+    };
+  },
+}))
 @Table({ tableName: "file_operations", modelName: "file_operation" })
 class FileOperation extends ParanoidModel<
   InferAttributes<FileOperation>,
@@ -59,6 +109,52 @@ class FileOperation extends ParanoidModel<
 > {
   static eventNamespace = "fileOperations";
 
+  /**
+   * Overrides the standard findByPk behavior to allow loading the exported
+   * collection or document with memberships for a user passed in by `userId`.
+   *
+   * @param id uuid
+   * @param options FindOptions
+   * @returns a promise resolving to a file operation instance or null.
+   */
+  static async findByPk(
+    id: Identifier,
+    options?: NonNullFindOptions<FileOperation> & AdditionalFindOptions
+  ): Promise<FileOperation>;
+  static async findByPk(
+    id: Identifier,
+    options?: FindOptions<FileOperation> & AdditionalFindOptions
+  ): Promise<FileOperation | null>;
+  static async findByPk(
+    id: Identifier,
+    options: FindOptions<FileOperation> & AdditionalFindOptions = {}
+  ): Promise<FileOperation | null> {
+    if (typeof id !== "string") {
+      return null;
+    }
+
+    const { userId, ...rest } = options;
+
+    // Preserve the caller's scope when no userId is passed
+    const scope = userId
+      ? this.scope({ method: ["withSource", userId] })
+      : this;
+
+    const fileOperation = await scope.findOne({
+      ...rest,
+      where: { id },
+      rejectOnEmpty: false,
+    });
+
+    if (!fileOperation && rest.rejectOnEmpty) {
+      throw rest.rejectOnEmpty instanceof Error
+        ? rest.rejectOnEmpty
+        : new EmptyResultError(`File operation doesn't exist with id: ${id}`);
+    }
+
+    return fileOperation;
+  }
+
   @Column(DataType.ENUM(...Object.values(FileOperationType)))
   type: FileOperationType;
 

server/policies/collection.ts

@@ -1,7 +1,7 @@
 import invariant from "invariant";
-import { CollectionPermission } from "@shared/types";
+import { CollectionPermission, TeamPreference } from "@shared/types";
 import { Collection, User, Team } from "@server/models";
-import { allow } from "./cancan";
+import { allow, can } from "./cancan";
 import { and, isTeamAdmin, isTeamModel, isTeamMutable, or } from "./utils";
 
 allow(User, "createCollection", Team, (actor, team) =>
@@ -46,6 +46,16 @@ allow(User, "read", Collection, (user, collection) => {
   return true;
 });
 
+allow(User, "download", Collection, (actor, collection) =>
+  and(
+    can(actor, "read", collection),
+    or(
+      and(!actor.isGuest, !actor.isViewer),
+      !!actor.team.getPreference(TeamPreference.ViewersCanExport)
+    )
+  )
+);
+
 allow(
   User,
   ["readDocument", "star", "unstar", "subscribe", "unsubscribe"],
@@ -174,7 +184,7 @@ allow(
     )
 );
 
-allow(User, ["update", "export", "archive"], Collection, (user, collection) =>
+allow(User, ["update", "archive"], Collection, (user, collection) =>
   and(
     !!collection,
     !!collection?.isActive,
@@ -185,6 +195,18 @@ allow(User, ["update", "export", "archive"], Collection, (user, collection) =>
   )
 );
 
+allow(User, "export", Collection, (user, collection) =>
+  and(
+    !!collection,
+    !!collection?.isActive,
+    can(user, "download", collection),
+    or(
+      isTeamAdmin(user, collection),
+      includesMembership(collection, [CollectionPermission.Admin])
+    )
+  )
+);
+
 allow(User, "delete", Collection, (user, collection) =>
   and(
     !!collection,

server/policies/fileOperation.ts

@@ -1,6 +1,6 @@
 import { FileOperationState, FileOperationType } from "@shared/types";
 import { User, Team, FileOperation } from "@server/models";
-import { allow } from "./cancan";
+import { allow, can } from "./cancan";
 import { and, isTeamAdmin, isTeamModel, isTeamMutable, or } from "./utils";
 
 const TerminalStates = [
@@ -9,6 +9,25 @@ const TerminalStates = [
   FileOperationState.Expired,
 ];
 
+/**
+ * An export is a snapshot that outlives the permissions it was created under,
+ * so access to the source is re-checked rather than trusting the ownership of
+ * the file operation alone.
+ */
+function canAccessSource(actor: User, fileOperation: FileOperation) {
+  if (fileOperation.type !== FileOperationType.Export) {
+    return true;
+  }
+  if (fileOperation.documentId) {
+    return can(actor, "download", fileOperation.document);
+  }
+  if (fileOperation.collectionId) {
+    return can(actor, "download", fileOperation.collection);
+  }
+  // Exports without a collection or document cover the entire workspace.
+  return can(actor, "createExport", actor.team);
+}
+
 allow(
   User,
   ["createFileOperation", "createExport"],
@@ -20,7 +39,13 @@ allow(
 allow(User, "read", FileOperation, (actor, fileOperation) =>
   and(
     isTeamModel(actor, fileOperation),
-    or(isTeamAdmin(actor, fileOperation), fileOperation?.userId === actor.id)
+    or(
+      isTeamAdmin(actor, fileOperation),
+      and(
+        fileOperation?.userId === actor.id,
+        !!fileOperation && canAccessSource(actor, fileOperation)
+      )
+    )
   )
 );
 

server/routes/api/fileOperations/fileOperations.test.ts

@@ -1,8 +1,19 @@
-import { FileOperationState, FileOperationType } from "@shared/types";
-import { Collection, User, Event, FileOperation } from "@server/models";
+import {
+  CollectionPermission,
+  FileOperationState,
+  FileOperationType,
+} from "@shared/types";
+import {
+  Collection,
+  User,
+  Event,
+  FileOperation,
+  UserMembership,
+} from "@server/models";
 import {
   buildAdmin,
   buildCollection,
+  buildDocument,
   buildFileOperation,
   buildTeam,
   buildUser,
@@ -37,6 +48,47 @@ describe("#fileOperations.info", () => {
   });
 
   it("should allow user to read their own export", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({ teamId: team.id });
+    const exportData = await buildFileOperation({
+      type: FileOperationType.Export,
+      teamId: team.id,
+      userId: user.id,
+      collectionId: collection.id,
+    });
+    const res = await server.post("/api/fileOperations.info", user, {
+      body: {
+        id: exportData.id,
+      },
+    });
+    const body = await res.json();
+    expect(res.status).toEqual(200);
+    expect(body.data.id).toBe(exportData.id);
+  });
+
+  it("should not allow user to read their own export of a collection they lost access to", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const exportData = await buildFileOperation({
+      type: FileOperationType.Export,
+      teamId: team.id,
+      userId: user.id,
+      collectionId: collection.id,
+    });
+    const res = await server.post("/api/fileOperations.info", user, {
+      body: {
+        id: exportData.id,
+      },
+    });
+    expect(res.status).toEqual(403);
+  });
+
+  it("should not allow demoted admin to read their own workspace export", async () => {
     const team = await buildTeam();
     const user = await buildUser({ teamId: team.id });
     const exportData = await buildFileOperation({
@@ -49,9 +101,7 @@ describe("#fileOperations.info", () => {
         id: exportData.id,
       },
     });
-    const body = await res.json();
-    expect(res.status).toEqual(200);
-    expect(body.data.id).toBe(exportData.id);
+    expect(res.status).toEqual(403);
   });
 
   it("should not allow user to read another user's export", async () => {
@@ -307,11 +357,13 @@ describe("#fileOperations.redirect", () => {
   it("should allow user to redirect their own export", async () => {
     const team = await buildTeam();
     const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({ teamId: team.id });
     const exportData = await buildFileOperation({
       state: FileOperationState.Complete,
       type: FileOperationType.Export,
       teamId: team.id,
       userId: user.id,
+      collectionId: collection.id,
     });
     const res = await server.post("/api/fileOperations.redirect", user, {
       body: {
@@ -322,6 +374,110 @@ describe("#fileOperations.redirect", () => {
     expect(res.status).toEqual(302);
   });
 
+  it("should not allow user to redirect their own export of a collection they lost access to", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const exportData = await buildFileOperation({
+      state: FileOperationState.Complete,
+      type: FileOperationType.Export,
+      teamId: team.id,
+      userId: user.id,
+      collectionId: collection.id,
+    });
+    const res = await server.post("/api/fileOperations.redirect", user, {
+      body: {
+        id: exportData.id,
+      },
+      redirect: "manual",
+    });
+    expect(res.status).toEqual(403);
+  });
+
+  it("should allow user to redirect their own export of a private collection they are a member of", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    await UserMembership.create({
+      createdById: user.id,
+      collectionId: collection.id,
+      userId: user.id,
+      permission: CollectionPermission.Admin,
+    });
+    const exportData = await buildFileOperation({
+      state: FileOperationState.Complete,
+      type: FileOperationType.Export,
+      teamId: team.id,
+      userId: user.id,
+      collectionId: collection.id,
+    });
+    const res = await server.post("/api/fileOperations.redirect", user, {
+      body: {
+        id: exportData.id,
+      },
+      redirect: "manual",
+    });
+    expect(res.status).toEqual(302);
+  });
+
+  it("should allow user to redirect their own document export", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({ teamId: team.id });
+    const document = await buildDocument({
+      teamId: team.id,
+      userId: user.id,
+      collectionId: collection.id,
+    });
+    const exportData = await buildFileOperation({
+      state: FileOperationState.Complete,
+      type: FileOperationType.Export,
+      teamId: team.id,
+      userId: user.id,
+      documentId: document.id,
+    });
+    const res = await server.post("/api/fileOperations.redirect", user, {
+      body: {
+        id: exportData.id,
+      },
+      redirect: "manual",
+    });
+    expect(res.status).toEqual(302);
+  });
+
+  it("should not allow user to redirect their own document export after losing access", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const document = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    const exportData = await buildFileOperation({
+      state: FileOperationState.Complete,
+      type: FileOperationType.Export,
+      teamId: team.id,
+      userId: user.id,
+      documentId: document.id,
+    });
+    const res = await server.post("/api/fileOperations.redirect", user, {
+      body: {
+        id: exportData.id,
+      },
+      redirect: "manual",
+    });
+    expect(res.status).toEqual(403);
+  });
+
   it("should not allow user to redirect another user's export", async () => {
     const team = await buildTeam();
     const admin = await buildAdmin({ teamId: team.id });

server/routes/api/fileOperations/fileOperations.ts

@@ -24,6 +24,7 @@ router.post(
     const { user } = ctx.state.auth;
 
     const fileOperation = await FileOperation.findByPk(id, {
+      userId: user.id,
       rejectOnEmpty: true,
     });
 
@@ -77,7 +78,8 @@ const handleFileOperationsRedirect = async (
   const id = (ctx.input.body.id ?? ctx.input.query.id) as string;
   const { user } = ctx.state.auth;
 
-  const fileOperation = await FileOperation.unscoped().findByPk(id, {
+  const fileOperation = await FileOperation.findByPk(id, {
+    userId: user.id,
     rejectOnEmpty: true,
   });
   authorize(user, "read", fileOperation);