Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-85162: Add HTTPSServer to http.server #129607
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
Conversation
donbarbos commented Feb 3, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
picnixz left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry but I can't review what is new and necessary from what is not (even with separate commits). While the style is inconsistent, let's address it in a separate PR if it's really that disturbing.
Note that since we sqash-merge at the end, the style changes will be part of the diff as well making the history harder to review.
Uh oh!
There was an error while loading. Please reload this page.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
This reverts commit b382985.
donbarbos commented Feb 3, 2025
I have made the requested changes; please review again |
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
picnixz left a comment
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.
The interface doesn't look the nicest to me. Is there an obvious reason why we can't add SSL support to HTTPServer even though it's named HTTPServer? Or should we really make HTTPSServer inherit from HTTPServer?
I am travelling and it's hard to review on mobile but @gpshead may have some preliminary comments on the design. I've left some comments but they can be disregarded for now until the design is fixed.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
donbarbos commented Feb 3, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@picnixz I don't want to change the HTTP server interface and overcomplicate it. I also think this matches the principle of separation of responsibilities. but I'm open to discussion |
picnixz commented Feb 3, 2025
Ok for a separate class but whether it should be a subclass or not is something I don't know if we should do. I don't know which layout will be the best one in the long term (for now let's keep it as a subclass). I don't think I have thought enough about the broader implications so I will hold future reviews for now. I think I need another opinion on this interface (btw, I'm not sure tiran is still active actually, hence the reason why I requested gpshead's opinion). |
gpshead left a comment
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.
regardless of review, I still wonder if we actually want this in the stdlib? But the implementation is not complicated so I don't think it'll be a maintenance burden in that regard.
The challenge is it may add more reasons for people to file security issues. Despite our existing bold warning up top in https://docs.python.org/3.14/library/http.server.html that http.server is not a production quality secure server.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
donbarbos commented Feb 3, 2025
I think it's important to add this because there are many useful use cases. And I don't see any reason why they would open an issue if they were already familiar with the concept of |
donbarbos commented Mar 7, 2025
@gpshead what do you think now about the merge of this PR? |
picnixz left a comment
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.
Sorry, but I missed one possible case
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
picnixz left a comment
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.
Just an improved blurb/what's new and I think it's good to go.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Library/2025-02-02-00-30-09.gh-issue-85162.BNF_aJ.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
picnixz commented Apr 4, 2025
I'll let it open a few days so that @gpshead can have a look, else I'll merge this before the end of the week. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
37bc386 into python:mainUh oh!
There was an error while loading. Please reload this page.
donbarbos commented Apr 5, 2025
Thanks a lot everyone |
I decided to resume the work on the implementation of HTTPS server in
http.servermodule.I am ready to respond in time and make changes :-)
I'm opening a new PR because this #20923 had been clearly abandoned.
I took the liberty of defining a single style in
Lib/http/server.pyfile, otherwise everything would have been completely confusing.cc @tiran@remilapeyre@jaswdr
(who was involved with the previous PR)
📚 Documentation preview 📚: https://cpython-previews--129607.org.readthedocs.build/