fix(file-utils): remove direct export of clamav to avoid direct dep (#2247)

* fix(file-utils): remove direct export of clamav to avoid direct dep

0

* fix(file-utils): better schema handling for multipart request

-

* feat(file-utils): better stream handling

GH-0

* fix(file-utils): refactor code to reduce cognitive complexity

GH-0

* fix(file-utils): add a test case

GH-0

* fix(file-utils): add test for multiple validator chain

GH-0

* fix(file-utils): remove file on error from validators

GH-0

* fix(file-utils): refactor to reduce complexity

GH-0

* fix(ci-cd): refactor a bit more

GH-0

Author: akshatdubeysf

packages/file-utils/src/__tests__/integration/file-uploader.integration.ts

@@ -1,6 +1,7 @@
 import {DataObject} from '@loopback/repository';
 import {Client, expect, sinon} from '@loopback/testlab';
 import {FileUtilBindings} from '../..';
+import {MultipartModel} from './fixtures';
 import {ParentWithFile} from './fixtures/controllers/parent.controller';
 import {MulterConfigService} from './fixtures/services/multer-config.service';
 import {TestApp} from './fixtures/test.application';
@@ -10,7 +11,10 @@ describe('FileUploaderComponent', () => {
   let app: TestApp;
   let client: Client;
   let receiverStub: {
-    receive: sinon.SinonStub<[DataObject<ParentWithFile>], void>;
+    receive: sinon.SinonStub<
+      [DataObject<ParentWithFile & MultipartModel>],
+      void
+    >;
   };
   before('setupApplication', async () => {
     ({app, client} = await setupApplication());
@@ -255,7 +259,35 @@ describe('FileUploaderComponent', () => {
           )
           .field('name', 'testName')
           .expect(400);
-        expect(response.body.error.message).to.equal('Infected file: test.txt');
+        expect(response.body.error.message).to.equal(
+          'File is infected - test.txt',
+        );
+      });
+      it('should parse a multipart request and apply all the validators', async () => {
+        const response = await client
+          .post('/multiple')
+          .attach(
+            'files',
+            './src/__tests__/integration/fixtures/dummy-files/test.txt',
+          )
+          .field('name', 'testName')
+          .expect(400);
+        expect(response.body.error.message).to.equal(
+          'File is infected - test.txt',
+        );
+
+        await client
+          .post('/multiple')
+          .attach(
+            'files',
+            './src/__tests__/integration/fixtures/dummy-files/test.png',
+          )
+          .field('name', 'testName')
+          .expect(204);
+
+        expect(
+          receiverStub.receive.getCalls()[0].args[0].files?.size,
+        ).deepEqual(1916);
       });
     });
   });

packages/file-utils/src/__tests__/integration/fixtures/controllers/multipart.controller.ts

@@ -0,0 +1,24 @@
+import {inject} from '@loopback/core';
+import {getModelSchemaRef, post, response} from '@loopback/rest';
+import {multipartRequestBody} from '../../../../decorators';
+import {MultipartModel} from '../models';
+
+export class MultipartController {
+  constructor(
+    @inject('services.ReceiverStub')
+    private readonly receiverStub: {
+      receive: (data: MultipartModel) => void;
+    },
+  ) {}
+  @post(`/multiple`)
+  @response(200, {
+    description: 'Multipart model instance',
+    content: {'application/json': {schema: getModelSchemaRef(MultipartModel)}},
+  })
+  async createMultiple(
+    @multipartRequestBody(MultipartModel)
+    data: MultipartModel,
+  ): Promise<void> {
+    this.receiverStub.receive(data);
+  }
+}

packages/file-utils/src/__tests__/integration/fixtures/models/index.ts

@@ -1,3 +1,4 @@
+export * from './multipart.model';
 export * from './multiple-files.model';
 export * from './parent-with-config.model';
 export * from './parent.model';

packages/file-utils/src/__tests__/integration/fixtures/models/multipart.model.ts

@@ -0,0 +1,19 @@
+import {Entity, model} from '@loopback/repository';
+import {fileProperty} from '../../../../decorators/file-property.decorator';
+import {FileTypeValidator} from '../../../../services';
+import {ClamAVValidator} from '../../../../sub-packages';
+
+@model()
+export class MultipartModel extends Entity {
+  @fileProperty({
+    type: 'array',
+    itemType: 'string',
+    validators: [FileTypeValidator, ClamAVValidator],
+    extensions: ['.pdf', '.txt', '.png'],
+  })
+  files: Express.Multer.File;
+
+  constructor(data?: Partial<MultipartModel>) {
+    super(data);
+  }
+}

packages/file-utils/src/__tests__/integration/fixtures/test.application.ts

@@ -5,7 +5,7 @@ import {RestApplication} from '@loopback/rest';
 import {ServiceMixin} from '@loopback/service-proxy';
 import {AWSS3Bindings} from 'loopback4-s3';
 import {FileUtilComponent} from '../../../component';
-import {ClamAVValidator} from '../../../services';
+import {ClamAVValidator} from '../../../sub-packages';
 import {Parent} from './models';
 
 export class TestApp extends BootMixin(

packages/file-utils/src/constant.ts

@@ -1,3 +1,4 @@
+import {ReferenceObject, SchemaObject} from '@loopback/rest';
 import multer from 'multer';
 import {
   IFileRequestMetadata,
@@ -35,3 +36,13 @@ export function getUploadConfig(
     ? (uploadOptions as IFileRequestMetadata<never>)
     : metadata;
 }
+
+export function isSchemaObject(
+  ob: SchemaObject | ReferenceObject | undefined,
+): ob is SchemaObject {
+  return (ob as SchemaObject)?.type !== undefined;
+}
+
+export function isFile(file: Partial<File>) {
+  return file instanceof File;
+}

packages/file-utils/src/decorators/file-property.decorator.ts

@@ -11,5 +11,5 @@ import {IFileRequestMetadata} from '../types';
 export function fileProperty<T>(
   definition: Partial<PropertyDefinition & IFileRequestMetadata<T>>,
 ) {
-  return property(definition);
+  return property({...definition, file: true});
 }

packages/file-utils/src/decorators/multipart-request-body.decorator.ts

@@ -1,11 +1,30 @@
 import {inject, ParameterDecoratorFactory} from '@loopback/core';
-import {getJsonSchema, requestBody, SchemaObject} from '@loopback/rest';
+import {
+  getJsonSchema,
+  ReferenceObject,
+  requestBody,
+  SchemaObject,
+} from '@loopback/rest';
 import {cloneDeep} from 'lodash';
 
 import {AnyObject, Entity} from '@loopback/repository';
 import {ModelConstructor} from '@sourceloop/core';
+import {isSchemaObject} from '../constant';
 import {FileUtilBindings} from '../keys';
 import {IModelWithFileMetadata} from '../types';
+
+function updateSchema(fieldSchema: SchemaObject | ReferenceObject | undefined) {
+  if (isSchemaObject(fieldSchema)) {
+    if (fieldSchema.type === 'array') {
+      fieldSchema.items = {
+        ...fieldSchema.items,
+        format: 'binary',
+      };
+    } else {
+      fieldSchema.format = 'binary';
+    }
+  }
+}
 /**
  * Decorator for handling multipart/form-data requests with file uploads and JSON data.
  *
@@ -49,12 +68,7 @@ export function multipartRequestBody<S extends Entity, T = AnyObject>(
   return function (target: object, member: string, index: number) {
     const defaultSchema: SchemaObject = {
       type: 'object',
-      properties: {
-        file: {
-          type: 'string',
-          format: 'binary',
-        },
-      },
+      properties: {},
     };
     const schema = getJsonSchema(model) as SchemaObject;
     const fileFields = Object.entries(model.definition.properties)
@@ -63,12 +77,11 @@ export function multipartRequestBody<S extends Entity, T = AnyObject>(
     if (schema && fileFields.length) {
       const newSchema = cloneDeep(schema);
       for (const field of fileFields) {
-        if (newSchema.properties?.[field]) {
-          (newSchema.properties?.[field] as SchemaObject).format = 'binary';
-        }
+        const fieldSchema = newSchema.properties?.[field];
+        updateSchema(fieldSchema);
       }
       defaultSchema.properties = {
-        ...defaultSchema.properties,
+        ...schema.properties,
         ...newSchema.properties,
       };
     }

packages/file-utils/src/services/file-validator.provider.ts

@@ -1,10 +1,11 @@
-import {Getter, extensionPoint, extensions, inject} from '@loopback/core';
+import {extensionPoint, extensions, Getter, inject} from '@loopback/core';
 import {FileUtilBindings, FileValidatorExtensionPoint} from '../keys';
 import {
   FileValidatorWithConstructor,
   getConfigProperty,
   IFileRequestMetadata,
   ParsedMultipartData,
+  ValidationResult,
 } from '../types';
 
 @extensionPoint(FileValidatorExtensionPoint.key)
@@ -17,7 +18,7 @@ export class FileValidatorService {
   ) {}
   async validateParsedData(
     parsed: ParsedMultipartData,
-  ): Promise<Express.Multer.File | undefined> {
+  ): Promise<ValidationResult | undefined> {
     const {file} = parsed;
     if (!file) {
       return file;
@@ -31,9 +32,17 @@ export class FileValidatorService {
         applicable.includes(validator.constructor),
       );
     }
+    const waiters: Promise<string | null>[] = [];
     for (const validator of filteredValidators) {
-      newFile = await validator.validate(newFile);
+      const result = await validator.validate(newFile);
+      newFile = result.file;
+      if (result.waiter) {
+        waiters.push(result.waiter);
+      }
     }
-    return newFile;
+    return {
+      file: newFile,
+      waiters,
+    };
   }
 }

packages/file-utils/src/services/multer-storage.provider.ts

@@ -1,8 +1,12 @@
-import {Context, Provider, inject, service} from '@loopback/core';
+import {Context, inject, Provider, service} from '@loopback/core';
+import {HttpErrors, Request} from '@loopback/rest';
 import multer from 'multer';
-import {getConfigProperty, IFileRequestMetadata} from '../types';
 import {FileUtilBindings} from '../keys';
-import {Request} from '@loopback/rest';
+import {
+  getConfigProperty,
+  IFileRequestMetadata,
+  ValidationResult,
+} from '../types';
 import {FileValidatorService} from './file-validator.provider';
 
 export class MulterStorageProvider implements Provider<multer.StorageEngine> {
@@ -28,8 +32,19 @@ export class MulterStorageProvider implements Provider<multer.StorageEngine> {
             file,
             body: req.body,
           })
-          .then(newFile => {
-            if (newFile) storage._handleFile(req, newFile, cb);
+          .then(result => {
+            if (result) {
+              storage._handleFile(req, result.file, (error, info) => {
+                this._handleValidationResults(
+                  result,
+                  info,
+                  storage,
+                  req,
+                  cb,
+                  error,
+                );
+              });
+            }
           })
           .catch(err => cb(err));
       },
@@ -50,4 +65,39 @@ export class MulterStorageProvider implements Provider<multer.StorageEngine> {
       `services.${config?.storageClass.name ?? 'MulterMemoryStorage'}`,
     );
   }
+
+  private _handleValidationResults(
+    result: ValidationResult,
+    parsedFile: Partial<Express.Multer.File> | undefined,
+    storage: multer.StorageEngine,
+    req: Request,
+    cb: (error?: Error, info?: Partial<Express.Multer.File>) => void,
+    error: Error | undefined,
+  ) {
+    if (error) {
+      cb(error);
+    } else {
+      Promise.all(result.waiters)
+        .then(results => {
+          const firstError = results.find(r => r);
+          if (firstError) {
+            if (parsedFile) {
+              // need to do this casting because of wrong typings
+              storage._removeFile(
+                req,
+                parsedFile as Express.Multer.File,
+                err => {
+                  cb(new HttpErrors.BadRequest(firstError));
+                },
+              );
+            } else {
+              cb(new HttpErrors.BadRequest(firstError));
+            }
+          } else {
+            cb(undefined, parsedFile);
+          }
+        })
+        .catch(cb);
+    }
+  }
 }