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

Multipart parser not standard compliant #4

Closed
defnull opened this issue Jan 30, 2025 · 6 comments
Closed

Multipart parser not standard compliant #4

defnull opened this issue Jan 30, 2025 · 6 comments

Comments

@defnull
Copy link

defnull commented Jan 30, 2025

The multipart parser fails to parse valid multipart requests, and happily parses invalid multipart requests. It does not follow the actual syntax rules of multipart/form+data (HTML5, RFC-7578) which can result in incomplete or incorrect data, or even security issues. It also requires the entire request body to be loaded into memory (multiple times) and is thus not suitable for large file uploads. Clients uploading more data than there is memory available will crash the application.

@patx
Copy link
Owner

patx commented Jan 31, 2025

This, I am still thinking about, it seems the best way would be to use your multipart library. However that adds a dependency which I am/was trying to avoid. I'll take a look at the RFC again and see what I come up with. Again thank you for pointing this out, I really appreciate your help and feedback!!

EDIT: I just saw you guys have done this with bottle while maintaining its portable single file forma, very cool. Will definitely look more into it.

@defnull
Copy link
Author

defnull commented Jan 31, 2025

Bottle uses a blocking parser, which is simpler and smaller than a non-blocking parser you most likely want to use in an ASGI environment. And it's not pretty. Building a no-dependency web framework is really annoying, let me tell you ;)

@defnull
Copy link
Author

defnull commented Jan 31, 2025

Maybe make the dependency optional? Just don't support multipart (and file uploads) if multipart is not installed. Most small apps don't need it, form submissions without file uploads should always be sent as url-encoded anyway.

@patx
Copy link
Owner

patx commented Jan 31, 2025

I was thinking hard on this as I was going to sleep last night and I kept coming back to the exact same idea you mentioned, just take it out completely unless the user has have multipart installed. I will implement today. We already do this with jinja2 so I think this is probably the best solution for now at least. Thanks for mentioning this as it validated my bed time ponderings haha!

@patx
Copy link
Owner

patx commented Feb 2, 2025

@defnull I have added your multipart parser! Thanks for the suggestion! I will close this issue out if no one finds anything wrong with the integration as this is a major change.

@patx
Copy link
Owner

patx commented Feb 4, 2025

Just an update for anyone following along, may even bring in aiofiles in this method to make it fully async, currently it blocks when opening/creating the new empty file for writing. As it is right now I dont think it will affect performance to much but should be improved eventually.

@patx patx closed this as completed Feb 4, 2025
patx pushed a commit that referenced this issue Feb 6, 2025
…edirect-m6mmz2bx

⚡️ Speed up method `Server._redirect` by 9%
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants