cdimascio / express-openapi-validator

🦋 Auto-validates api requests, responses, and securities using ExpressJS and an OpenAPI 3.x specification
MIT License
920 stars 211 forks source link

Multer worked on 5.0.1 throws internal server error in 5.1.0 #875

Closed tflolo closed 12 months ago

tflolo commented 1 year ago

Describe the bug I upgraded to v5.1.0 from v5.0.1 to get the fix where multipartMiddleware is put after securityMiddleware. When i did this, the file is uploaded, but i get this error:

{ "message": "Cannot read properties of undefined (reading 'file')" }

And I no longer get to my requestHandler to handle req.files and furher work with the data.

To Reproduce Upgrade from 5.0.1 to 5.1.0

Actual behavior File gets uploaded, but throws an internal server error that stops the request from getting to my requesthandler

Expected behavior Expected the req.files to be populated with data about my uploaded files.

Examples and context jest tets with supertest: openApi def:

post:
  tags:
    - Files
  summary: Upload file
  description: Upload a file
  requestBody:
    required: true
    content:
      multipart/form-data:
        schema:
          oneOf:
            - title: Multiple files
              type: object
              required:
                - files
              properties:
                files:
                  type: array
                  items:
                    type: string
                    format: binary
            - title: File
              type: object
              required:
                - file
              properties:
                file:
                  type: string
                  format: binary
  responses:
    201:
      description: File successfully uploaded
      content:
        application/json:
          schema:
            type: array
            items:
              required:
                - id
              properties:
                id:
                  allOf:
                    - $ref: ../../common/schemas/id.yaml
                  description: Id of File
    default:
      $ref: ../../common/responses/defaultError.yaml
  x-eov-operation-handler: upload.controller
  x-eov-operation-id: uploadFileRequest
        it('should upload one file and return 201 and the id of the file', async () => {
            const filePath = path.join(__dirname, '../test_file.txt');

            const response = await supertest(app)
                .post(urlUndertest)
                .set('Cookie', cookie)
                .attach('file', filePath);
            expect(response.statusCode).toBe(201);
            expect(response.body).toBeInstanceOf(Array<{ id: string }>);
        });

Request handler:

export const uploadFileRequest = async (req: Request, res: Response, next: NextFunction) => {
    try {
        console.log(req.files);
        return res.status(201).json(await uploadFiles(req.files as Express.Multer.File[]));
    } catch (error: unknown) {
        return next(error);
    }
};

On v5.0.1

I get

  console.log
    [
      {
        fieldname: 'file',
        originalname: 'test_file.txt',
        encoding: '7bit',
        mimetype: 'text/plain',
        destination: '/home/thomas/tmp/uploads',
        filename: 'c5f910ddcdc142b9692668da80e94d5a',
        path: '/home/thomas/tmp/uploads/c5f910ddcdc142b9692668da80e94d5a',
        size: 36167
      }
    ]

Only changing to v5.1.0 i get

{
    "message": "Cannot read properties of undefined (reading 'file')"
}

and of course not coming to my requestHandler, the file still uploads.

mdmower commented 1 year ago

@tflolo - Do any of your security handlers depend on form values submitted as part of the multipart/form-data request? From the OpenAPI schema you posted, it doesn't look this this would be the case, unless you removed those values before pasting here?

tflolo commented 1 year ago

@mdmower - None of my security handlers does anything other than check for a valid session. The code i posted is complete, and only thing i change between runs is the version of express-openapi-validator from 5.0.1 to 5.1.0. I can try to move multipartMiddlware after securityMiddleware in 5.0.1 and see if that works.

mdmower commented 12 months ago

@tflolo - Is there a chance that you recently updated your openapi schema to support multiple uploads via oneOf schema? I was able to reproduce the same error as you, but the error I found also exists in version 5.0.5. To see if we're observing the same error, when version 5.1.0 is installed, could you temporarily update the following file and retest?

in node_modules/express-openapi-validator/dist/middlewares/openapi.request.validator.js, replace old line 131:

const type = (_c = (_b = (_a = schemaBody === null || schemaBody === void 0 ? void 0 : schemaBody.properties) === null || _a === void 0 ? void 0 : _a.body) === null || _b === void 0 ? void 0 : _b.properties[key]) === null || _c === void 0 ? void 0 : _c.type;

with new line:

const type = (_d = (_c = (_b = (_a = schemaBody === null || schemaBody === void 0 ? void 0 : schemaBody.properties) === null || _a === void 0 ? void 0 : _a.body) === null || _b === void 0 ? void 0 : _b.properties) === null || _c === void 0 ? void 0 : _c[key]) === null || _d === void 0 ? void 0 : _d.type;

I've drafted PR #878 (which outputs the compiled JS above) but would like confirmation that it resolves the issue.

UPDATE: I just noticed that your report says you updated from 5.0.1 to 5.1.0, so this is starting to make sense. Support for nested JSON in multipart bodies was added in 5.0.2. See https://github.com/cdimascio/express-openapi-validator/commit/e5cb5d68607fa0ad37c074363c471bb11d7791bb .

tflolo commented 12 months ago

@mdmower The error changed,

      status: 500,
      text: '{"message":"_d is not defined"}',
      method: 'POST',
      path: '/api/wave-log/upload'

I upgraded to 5.1.0 and copied the line 131 you posted

UPDATE: _d was not defined in the function, i defined it and now it works :)

mdmower commented 12 months ago

UPDATE: _d was not defined in the function, i defined it and now it works :)

Thanks for fixing my derp. I'll request a review on the PR.