-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add support for completely disabling multipart request handling #66
base: develop
Are you sure you want to change the base?
Add support for completely disabling multipart request handling #66
Conversation
Documentation PR: adonisjs/v6-docs#191 |
throw new Exception('request content-type not supported', { | ||
status: 415, | ||
code: 'E_REQUEST_UNSUPPORTED_MEDIA_TYPE', | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to create a custom exception class for this and expose it via an errors
export, like the @adonisjs/auth
package?
That way, people can use instanceof errors....
in an error handler.
However, this would require new documentation for the new exception type...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the pattern already in use here: https://github.com/adonisjs/bodyparser/blob/develop/src/bodyparser_middleware.ts#L72-L89
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see! Thank you for clarifying!
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 |
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. |
Writing files to disk can be avoided by simply not processing the multipart files using 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 |
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 totrue
, 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 theAccept-Post
/Accept-Patch
header to indicate which content-types are supported by the server, however, this would need something specific inException
class from poppins.🔗 Linked issue
Discussed with @thetutlage on discord today.
❓ Type of 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