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

multipartMiddleware preceeds securityMiddleware so files begin uploading before security check #865

Closed mdmower-csnw closed 1 year ago

mdmower-csnw commented 1 year ago

Describe the bug

When OpenApiValidator.middleware() is configured to apply both upload and security middleware...

OpenApiValidator.middleware({
  ...
  fileUploader: {
    storage: myDiskStorageEngine,
  },
  validateSecurity: {
    handlers: {
      securityCheck: mySecurityCheck,
    },
  },
})

... the upload middleware (multipartMiddleware) runs before the security middleware (securityMiddleware). As a result, file uploads begin even for users that fail the security check.

The order of the middlewares is defined here: https://github.com/cdimascio/express-openapi-validator/blob/bb8d6b80c4bc42be9213d03c46b5d3f0195d1040/src/openapi.validator.ts#L152-L179

To Reproduce

  1. Configure OpenApiValidator.middleware with both a fileUploader.storage storage engine and a validateSecurity handler.
  2. Attempt to upload a file using multipart/form-data with a request that does not satisfy validateSecurity handler.

Actual behavior File upload will be processed. For example, if the multer storage engine is a disk storage engine, the file is written to disk.

Expected behavior The security check should abort the request pipeline before the file upload is processed.

Examples and context I've tested reversing the order of the above mentioned middleware locally by editing node_modules/express-openapi-validator/dist/openapi.validator.js and the security check works as expected (file upload does not begin). I am willing to create a PR for this change, but am unsure whether there's a reason for the current order (maybe the security check for some users depends on form data?). If so, perhaps the order could be configurable?

cdimascio commented 1 year ago

PR is welcome. The updated order makes sense.

mdmower-csnw commented 1 year ago

@cdimascio - I don't mean to be bothersome, but I'm hoping you could advise whether the timeline is days, weeks, or longer before a new release is prepared with a fix for this issue (PR #866). If you don't expect to have time for code review + release in the near future, I'll need to fork and temporarily point packages.json to the fork. Sorry for the nudge; just need to figure out my path forward for those who are expecting work from me.

cdimascio commented 1 year ago

available in v5.1.0