From e18e61000de01c1cd1d6d648e1223c6cb51a7f82 Mon Sep 17 00:00:00 2001 From: Ahmed Bouhuolia Date: Fri, 15 May 2026 14:47:18 +0200 Subject: [PATCH] fix(server): prevent cross-tenant attachment access (IDOR) Add tenant-scoped document lookup with throwIfNotFound() before S3 operations in GetAttachment, DeleteAttachment, and GetAttachmentPresignedUrl services. This prevents users from reading, deleting, or generating presigned URLs for attachments belonging to other tenants. Also adds RequirePermission decorators to the three attachment endpoints and introduces Attachment ability subject with View and Delete actions. GHSA-rc4v-wq22-v6cf Co-Authored-By: Claude Sonnet 4.6 --- .../modules/Attachments/Attachments.controller.ts | 6 ++++++ .../src/modules/Attachments/Attachments.types.ts | 5 +++++ .../src/modules/Attachments/DeleteAttachment.ts | 10 +++++----- .../server/src/modules/Attachments/GetAttachment.ts | 12 +++++++++++- .../modules/Attachments/GetAttachmentPresignedUrl.ts | 5 ++++- packages/server/src/modules/Roles/AbilitySchema.ts | 9 +++++++++ packages/server/src/modules/Roles/Roles.types.ts | 3 ++- 7 files changed, 42 insertions(+), 8 deletions(-) diff --git a/packages/server/src/modules/Attachments/Attachments.controller.ts b/packages/server/src/modules/Attachments/Attachments.controller.ts index cc431ed66..29a643f18 100644 --- a/packages/server/src/modules/Attachments/Attachments.controller.ts +++ b/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); diff --git a/packages/server/src/modules/Attachments/Attachments.types.ts b/packages/server/src/modules/Attachments/Attachments.types.ts index d1699ed12..a9d9644c2 100644 --- a/packages/server/src/modules/Attachments/Attachments.types.ts +++ b/packages/server/src/modules/Attachments/Attachments.types.ts @@ -1,3 +1,8 @@ export interface AttachmentLinkDTO { key: string; } + +export enum AttachmentAction { + View = 'View', + Delete = 'Delete', +} diff --git a/packages/server/src/modules/Attachments/DeleteAttachment.ts b/packages/server/src/modules/Attachments/DeleteAttachment.ts index dea4adb35..e278578da 100644 --- a/packages/server/src/modules/Attachments/DeleteAttachment.ts +++ b/packages/server/src/modules/Attachments/DeleteAttachment.ts @@ -31,17 +31,17 @@ export class DeleteAttachment { * @param {string} filekey */ async delete(filekey: string): Promise { + 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() diff --git a/packages/server/src/modules/Attachments/GetAttachment.ts b/packages/server/src/modules/Attachments/GetAttachment.ts index 5c448200a..6aff2624d 100644 --- a/packages/server/src/modules/Attachments/GetAttachment.ts +++ b/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,13 +12,21 @@ export class GetAttachment { @Inject(S3_CLIENT) private readonly s3: S3Client, + + @Inject(DocumentModel.name) + private readonly documentModel: TenantModelProxy, ) {} - + /** * Retrieves data of the given document key. * @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, diff --git a/packages/server/src/modules/Attachments/GetAttachmentPresignedUrl.ts b/packages/server/src/modules/Attachments/GetAttachmentPresignedUrl.ts index 10536ca77..ae4b97de9 100644 --- a/packages/server/src/modules/Attachments/GetAttachmentPresignedUrl.ts +++ b/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'; diff --git a/packages/server/src/modules/Roles/AbilitySchema.ts b/packages/server/src/modules/Roles/AbilitySchema.ts index 36c35cf71..853dc8690 100644 --- a/packages/server/src/modules/Roles/AbilitySchema.ts +++ b/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 }, + ], + }, ]; /** diff --git a/packages/server/src/modules/Roles/Roles.types.ts b/packages/server/src/modules/Roles/Roles.types.ts index 258b76509..daef030cc 100644 --- a/packages/server/src/modules/Roles/Roles.types.ts +++ b/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 {