bodyparser
bodyparser copied to clipboard
adonisjs
Add support for completely disabling multipart request handling
As discussed on discord, by the multipart request handling always being enabled, there can be security implications. This adds a multipart.enabled option, which defaults to true, which allows disabling handling for multipart requests.
If multipart.enabled === false then a multipart request is rejected immediately with a HTTP 514 UNSUPPORTED_MEDIA_TYPE response. As mentioned in that documentation, we could also set the Accept-Post / Accept-Patch header to indicate which content-types are supported by the server, however, this would need something specific in Exception class from poppins.
🔗 Linked issue
Discussed with @thetutlage on discord today.
❓ Type of change
- [ ] 🐞 Bug fix (a non-breaking change that fixes an issue)
- [x] 👌 Enhancement (improving an existing functionality like performance)
- [x] ✨ New feature (a non-breaking change that adds functionality)
- [ ] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
📚 Description
This allows people using Adonis Framework to improve the security of their servers by allowing them to completely disable multipart request handling. Previously you needed a custom middleware to handle this.
📝 Checklist
- [ ] I have linked an issue or discussion. — n/a
- [x] I have updated the documentation accordingly.
A fundamental question. Should this change be specific to multipart requests? Or should it be that the bodyparser will throw error when an unsupported content-type is sent in the request?
For example: I make a request with application/xml content-type and the bodyparser is not configured to parse it, then it should throw error. The same logic can be applied to any content-type.
A fundamental question. Should this change be specific to multipart requests? Or should it be that the bodyparser will throw error when an unsupported content-type is sent in the request?
For example: I make a request with
application/xmlcontent-type and the bodyparser is not configured to parse it, then it should throw error. The same logic can be applied to any content-type.
This is really specific to multipart requests, because in order to process them, you end up writing temporary files to disk. That's a great vector for a denial of service attack if those files aren't consumed.
Whilst yes, you could have an acceptedContentTypes option, I would consider that a different feature because it doesn't solve the same problem.
Many services never need to accept multipart requests, especially with micro/small services.
This is really specific to multipart requests, because in order to process them, you end up writing temporary files to disk. That's a great vector for a denial of service attack if those files aren't consumed.
Writing files to disk can be avoided by simply not processing the multipart files using autoProcess: false.
I believe, we should not add too many ways to achieve the same outcome. Ideally, I would want BodyParser as a whole to disallow requests for unsupported types vs just multipart disallowing requests.
The solution can be achieved by not introducing any additional fields. We can collect all the supported types by concatenating the types array from each parser block and then we check if types falls in the allowed types (if not) we deny the request.
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Writing files to disk can be avoided by simply not processing the multipart files using
autoProcess: false.
This still leaves the requests hanging open, which is an attack surface (resource exhaustion) as far as I know.
I'm not sure this types collection mechanism is clear enough. I think it'd be much clearer to just have a "is this parse of the middleware enabled or not" boolean for each, if that's really necessary.
This still leaves the requests hanging open, which is an attack surface (resource exhaustion) as far as I know.
Do you have an example where the request stays open? I tried it right now and the response was sent and no body was parsed at all (infact no attempt to read the stream was made).
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request