Merge pull request #1093 from bigcapitalhq/fix/attachment-tenant-isolation

fix(server): prevent cross-tenant attachment access (IDOR)

packages/server/src/modules/Attachments/Attachments.controller.ts

@@ -31,6 +31,9 @@ import { AttachmentUploadPipeline } from './S3UploadPipeline';
 import { FileInterceptor } from '@/common/interceptors/file.interceptor';
 import { ConfigService } from '@nestjs/config';
 import { ApiCommonHeaders } from '@/common/decorators/ApiCommonHeaders';
+import { RequirePermission } from '@/modules/Roles/RequirePermission.decorator';
+import { AbilitySubject } from '@/modules/Roles/Roles.types';
+import { AttachmentAction } from './Attachments.types';
 
 @ApiTags('Attachments')
 @Controller('/attachments')
@@ -86,6 +89,7 @@ export class AttachmentsController {
   @ApiOperation({ summary: 'Get attachment by ID' })
   @ApiParam({ name: 'id', description: 'Attachment ID' })
   @ApiResponse({ status: 200, description: 'Returns the attachment file' })
+  @RequirePermission(AttachmentAction.View, AbilitySubject.Attachment)
   async getAttachment(
     @Res() res: Response,
     @Param('id') documentId: string,
@@ -112,6 +116,7 @@ export class AttachmentsController {
     status: 200,
     description: 'The document has been deleted successfully',
   })
+  @RequirePermission(AttachmentAction.Delete, AbilitySubject.Attachment)
   async deleteAttachment(@Param('id') documentId: string) {
     await this.attachmentsApplication.delete(documentId);
 
@@ -185,6 +190,7 @@ export class AttachmentsController {
     status: 200,
     description: 'Returns the presigned URL for the attachment',
   })
+  @RequirePermission(AttachmentAction.View, AbilitySubject.Attachment)
   async getAttachmentPresignedUrl(@Param('id') documentKey: string) {
     const presignedUrl =
       await this.attachmentsApplication.getPresignedUrl(documentKey);

packages/server/src/modules/Attachments/Attachments.types.ts

@@ -1,3 +1,8 @@
 export interface AttachmentLinkDTO {
   key: string;
 }
+
+export enum AttachmentAction {
+  View = 'View',
+  Delete = 'Delete',
+}

packages/server/src/modules/Attachments/DeleteAttachment.ts

@@ -31,17 +31,17 @@ export class DeleteAttachment {
    * @param {string} filekey
    */
   async delete(filekey: string): Promise<void> {
+    const foundDocument = await this.documentModel()
+      .query()
+      .findOne('key', filekey)
+      .throwIfNotFound();
+
     const params = {
       Bucket: this.configService.get('s3.bucket'),
       Key: filekey,
     };
     await this.s3Client.send(new DeleteObjectCommand(params));
 
-    const foundDocument = await this.documentModel()
-      .query()
-      .findOne('key', filekey)
-      .throwIfNotFound();
-
     await this.uow.withTransaction(async (trx: Knex.Transaction) => {
       // Delete all document links
       await this.documentLinkModel()

packages/server/src/modules/Attachments/GetAttachment.ts

@@ -2,6 +2,8 @@ import { GetObjectCommand, S3Client } from '@aws-sdk/client-s3';
 import { Inject, Injectable } from '@nestjs/common';
 import { ConfigService } from '@nestjs/config';
 import { S3_CLIENT } from '../S3/S3.module';
+import { TenantModelProxy } from '../System/models/TenantBaseModel';
+import { DocumentModel } from './models/Document.model';
 
 @Injectable()
 export class GetAttachment {
@@ -10,6 +12,9 @@ export class GetAttachment {
 
     @Inject(S3_CLIENT)
     private readonly s3: S3Client,
+
+    @Inject(DocumentModel.name)
+    private readonly documentModel: TenantModelProxy<typeof DocumentModel>,
   ) {}
 
   /**
@@ -17,6 +22,11 @@ export class GetAttachment {
    * @param {string} filekey
    */
   async getAttachment(filekey: string) {
+    await this.documentModel()
+      .query()
+      .findOne('key', filekey)
+      .throwIfNotFound();
+
     const params = {
       Bucket: this.configService.get('s3.bucket'),
       Key: filekey,

packages/server/src/modules/Attachments/GetAttachmentPresignedUrl.ts

@@ -24,7 +24,10 @@ export class GetAttachmentPresignedUrl {
    * @returns {string}
    */
   async getPresignedUrl(key: string) {
-    const foundDocument = await this.documentModel().query().findOne({ key });
+    const foundDocument = await this.documentModel()
+      .query()
+      .findOne({ key })
+      .throwIfNotFound();
     const config = this.configService.get('s3');
 
     let ResponseContentDisposition = 'attachment';

packages/server/src/modules/Roles/AbilitySchema.ts

@@ -16,6 +16,7 @@ import { BillAction } from "../Bills/Bills.types";
 import { AbilitySubject, ISubjectAbilitiesSchema, ISubjectAbilitySchema } from "./Roles.types";
 import { PaymentReceiveAction } from "../PaymentReceived/types/PaymentReceived.types";
 import { PreferencesAction } from "../Settings/Settings.types";
+import { AttachmentAction } from "../Attachments/Attachments.types";
 
 export const AbilitySchema: ISubjectAbilitiesSchema[] = [
   {
@@ -305,6 +306,14 @@ export const AbilitySchema: ISubjectAbilitiesSchema[] = [
       },
     ],
   },
+  {
+    subject: AbilitySubject.Attachment,
+    subjectLabel: 'ability.attachments',
+    abilities: [
+      { key: AttachmentAction.View, label: 'ability.view', default: true },
+      { key: AttachmentAction.Delete, label: 'ability.delete', default: true },
+    ],
+  },
 ];
 
 /**

packages/server/src/modules/Roles/Roles.types.ts

@@ -60,7 +60,8 @@ export enum AbilitySubject {
   CreditNote = 'CreditNode',
   VendorCredit = 'VendorCredit',
   Project = 'Project',
-  TaxRate = 'TaxRate'
+  TaxRate = 'TaxRate',
+  Attachment = 'Attachment',
 }
 
 export interface IRoleCreatedPayload {