-
-
Notifications
You must be signed in to change notification settings - Fork 99
Re-raise PyOpenSSL SysCallErrors as respective errno-mapped standard Python ConnectionError subclasses
#786
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
base: main
Are you sure you want to change the base?
Conversation
d147658 to
9618049
Compare
9618049 to
350bac4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #786 +/- ##
==========================================
+ Coverage 79.30% 79.32% +0.02%
==========================================
Files 29 30 +1
Lines 4203 4237 +34
Branches 539 539
==========================================
+ Hits 3333 3361 +28
- Misses 728 730 +2
- Partials 142 146 +4 |
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.
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.
is ok now?
e2f0a3b to
b594cd5
Compare
cbc0181 to
4444f9d
Compare
cheroot/ssl/pyopenssl.py
Outdated
| # translate any SysCallError to ConnectionError | ||
| with _morph_syscall_to_connection_error(method): | ||
| return getattr(self._ssl_conn, method)(*new_args) |
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 guess this is one way of solving that.. we could do so for now. Although, I'd be good to get rid of SSLConnectionProxyMeta in some future iteration/PR. I've always found this metaclass patching confusing and wanted to simplify it if possible.
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 tried refactoring to remove the metaclass and I think it works but will add as a new PR.
SysCallErrors as respective errno-mapped standard Python ConnectionError subclasses
4444f9d to
8dc9dc5
Compare
f691222 to
b1308b5
Compare
|
|
||
| # Create SSLConnection object. The patched SSL.Connection() call returns | ||
| # a mock that is stored internally as conn._ssl_conn. | ||
| conn = SSLConnection(mock_ssl_context) |
Check failure
Code scanning / CodeQL
Non-callable called Error test
non-callable
class SSLConnectionProxyMeta
1dabdf8 to
b768b11
Compare
bf8e9e0 to
e9a31db
Compare
PyOpenSSL raises SysCallError which cheroot does not translate, leading to unhandled exceptions when the underlying connection is closed or shut down abruptly. This commit ensures all relevant errno codes are translated into standard Python ConnectionError types for reliable handling by consumers.
85afc8e to
a26fd31
Compare
Adds a more robust error-handling mechanism to the
PyOpenSSLSSLConnectionwrapper to better manage exceptions raised during connection shutdown and close operations.The
pyopenssl.pymodule now includes the context manager,_morph_syscall_to_connection_error, to wrap error-prone methods. This context manager intercepts low-levelOpenSSL.SSL.SysCallErrorexceptions and translates them into standard Python exception types (e.g.,ConnectionAbortedError,BrokenPipeError,ConnectionRefusedError, etc.) based on the underlyingerrnocode.Why this is useful: Currently, when a connection fails due to a system event (like a remote peer abruptly closing the socket), the resulting
SysCallErrorcan be difficult to catch and handle cleanly in application code.This change ensures that applications using Cheroot can reliably catch a standard
ConnectionErroror one of its subclasses, improving the robustness and usability of the SSL connection handling.This change is