search

bottlepy / bottle

bottle.py is a fast and simple micro-framework for python web-applications.
http://bottlepy.org/
MIT License
8.39k stars 1.47k forks source link

fix: cgi.FieldStorage not available in Python 3.13 #1438

Closed defnull closed 3 weeks ago

defnull commented 10 months ago

cgi.FieldStorage (used for multipart parsing) was deprecated in Python 3.11 and removed in 3.13. We now have to ship our own implementation (mostly a copy&paste from the multipart module)

oz123 commented 9 months ago

@defnull is there anyway one can help you to get this merged?

defnull commented 9 months ago

Tests do not actually pass yet and test coverage was lacking, so this should be considered a WIP for now and not ready to merge yet.

I had a local version that has better test coverage and kinda works, but was not pushed yet because I'm still not confident enough that this is a proper replacement. I'll push a working version now and would appreciate some testing (correctness and performance) and feedback.

defnull commented 9 months ago

Github actions are kinda useless for testing legacy python versions. I'll have to somehow get those tests working locally. We still want to support 2.7 and 3.6+ with the master branch.

oz123 commented 9 months ago

May out of scope here, but why do you still keen on supporting legacy python versions on the master branch? Also, I can help with fixing GH workflows on those python versions, if this helps brings this forward (hint, I'll use asdf to install python2.7 - python3.7 on a plain ubuntu container).

defnull commented 9 months ago

May out of scope here, but why do you still keen on supporting legacy python versions on the master branch?

Yes you are right, that should not be a blocking issue for the next release.

The 0.12 branch supports versions down to 2.5 and 3.2. An impressive feat and really nice for development in legacy environments, but no longer feasible with modern versions in mind. The master/main branch which should have been released years ago as 0.13 already dropped 2.5 and 2.6, which leaves 2.7 and 3.2+. But supporting 3.2 in a new release makes no sense anymore.

Maybe we should just move on, prepare Bottle 1.0 with a Python 3.8+ requirement and call it a day. People that need support for EOL Python versions can still rely on Bottle 0.12

oz123 commented 8 months ago

Ping. Do you need help with finishing this PR? This is almost done...

memsharded commented 1 month ago

Hello!

I'd like to ask about the plans for this PR, Python 3.13 is approaching (Oct 1st 2024, one month). Is there something I can do to help? I don't have the expertise for the fix itself, but as a user I could at least give it a try and provide some testing.

Thanks a lot for bottle and for trying to fix this.

defnull commented 1 month ago

The PR passes all tests now and also contains a documentation overhaul that was long overdue. I'll be off the computer for a couple of days and would like to ask everyone to have a look and provide feedback. I'm actually quite happy with it now and confident enough to consider merging. If there are no major issues, I'd try and merge this into main next week, do a bit more cleanup, and then prepare a 0.13 release.

This will be an 'emergency release' that does break existing applications running on ancient Python versions, but still supports Python >=2.7.3 and a lot of 3.x releases (tested with 3.8-3.12). That should be fine for 99% of users, and those that are affected can and should stay on 0.12.x forever. Dropping Python 2 support completely is the next step, but too much of a change for right now.

oz123 commented 1 month ago

Thank for your effort. Great last moment. Enjoy your time off, and please think of solving the human resource problem. Currently, you are the only developer activity working on bottle with access to the GitHub organization. Please consider asking tidelift for help, and involving at least one other person.

memsharded commented 1 month ago

Some early feedback about this change.

I have execute our test suite using this branch as source, 1330 unit tests, 2600 integration tests. Not all of them use the functionality, but many of the integration tests will be uploading and downloading files.

I have also launched our bottle-based server application and tested it manually, with some real world (larger than tests) transfers. Everything seems to be smooth here, nothing broken, everything seems to be working fine.

Thanks very much for your efforts again!

defnull commented 4 weeks ago

This kind of feedback helps a TON in gaining confidence for a release after such a long time. Thanks a lot!

defnull commented 4 weeks ago

Still found a bug. curl uses uppercase characters in boundaries but BaseRequest.content_type returns a lower-cased string, so the parser got initialized with a lower-case boundary and never found it in the input stream. This is something 'code coverage' cannot protect against.

defnull commented 4 weeks ago

Sooo... there is a potential DoS vector in the current implementation, which also affected cgi.FieldStorage. I have a solution ready, which also improves throughput by x2 to x10 and will be merged into multipart as soon as it is ready. We can ignore that for now (since this is not a regression) and replace the implementation later, no public APIs are affected.

memsharded commented 4 weeks ago

Sooo... there is a potential DoS vector in the current implementation, which also affected cgi.FieldStorage. I have a solution ready, which also improves throughput by x2 to x10 and will be merged into multipart as soon as it is ready. We can ignore that for now (since this is not a regression) and replace the implementation later, no public APIs are affected.

I think it is fair to keep it decoupled for now if it was already there in cgi and do a 1-1 replacement, then approach this as an improvement later. From my side, there is no concern about this potential DoS.