Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mdmower-csnw opened this issue Oct 12, 2023 · 3 comments · Fixed by #866 or CSNW/express-openapi-validator#1

Comments

@mdmower-csnw
Copy link
Contributor

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:

if (this.options.fileUploader) {
// multipart middleware
let fumw;
middlewares.push(function multipartMiddleware(req, res, next) {
return pContext
.then(({ context: { apiDoc } }) => {
fumw = fumw || self.multipartMiddleware(apiDoc);
return fumw(req, res, next);
})
.catch(next);
});
}
// security middlware
let scmw;
middlewares.push(function securityMiddleware(req, res, next) {
return pContext
.then(({ context: { apiDoc } }) => {
const components = apiDoc.components;
if (self.options.validateSecurity && components?.securitySchemes) {
scmw = scmw || self.securityMiddleware(apiDoc);
return scmw(req, res, next);
} else {
next();
}
})
.catch(next);
});

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
Copy link
Owner

PR is welcome. The updated order makes sense.

mdmower added a commit to mdmower/express-openapi-validator that referenced this issue Oct 13, 2023
- Move multipart middleware after security middleware so that security
  handlers can abort request pipeline before uploads are processed.

Fixes cdimascio#865
@mdmower-csnw
Copy link
Contributor Author

@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.

mdmower-csnw pushed a commit to CSNW/express-openapi-validator that referenced this issue Oct 20, 2023
- Move multipart middleware after security middleware so that security
  handlers can abort request pipeline before uploads are processed.

Fixes cdimascio#865
mdmower-csnw added a commit to CSNW/express-openapi-validator that referenced this issue Oct 20, 2023
- Move multipart middleware after security middleware so that security
  handlers can abort request pipeline before uploads are processed.

Fixes cdimascio#865

Co-authored-by: Matt Mower <[email protected]>
@mdmower-csnw mdmower-csnw reopened this Oct 28, 2023
cdimascio pushed a commit that referenced this issue Nov 12, 2023
- Move multipart middleware after security middleware so that security
  handlers can abort request pipeline before uploads are processed.

Fixes #865
@cdimascio
Copy link
Owner

available in v5.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants