-
-
Notifications
You must be signed in to change notification settings - Fork 98
Handle socket errors when sending HTTP-over-HTTPS error responses #802
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like that we mess with private attributes (although, the existing code already accesses it). In particular, it's weird how TLS quirks leak into makefile. I'd wait for other bits of refactoring, especially the removal of makefile in favor of classmethods before doing anything about this. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,26 @@ def write(self, val, *args, **kwargs): | |
| self.bytes_written += len(val) | ||
| return res | ||
|
|
||
| def _flush_unlocked(self): | ||
| """ | ||
| Flush buffered output to the underlying socket. | ||
|
|
||
| We override this method because when a client sends plaintext HTTP | ||
| to a TLS-only port, SSL detects a bad handshake and may | ||
| invalidate the underlying socket. At that point, the socket | ||
| may not be writable. Attempting to write (e.g., sending a | ||
| 400 Bad Request) may succeed or raise OSError. This override | ||
| prevents OSError from propagating. | ||
|
Comment on lines
+75
to
+80
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part looks like a code comment, not a docstring. |
||
| """ | ||
| try: | ||
| super()._flush_unlocked() | ||
| except OSError: | ||
| # The socket is already closed or otherwise unusable (e.g. TLS | ||
| # fatal alert triggered by invalid handshake). | ||
| # Clearing the buffer prevents the error from happening | ||
| # again during cleanup. | ||
| self._write_buf.clear() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if we should be suppressing problems on the low level like this. It's likely the caller that should perform the suppression since the stream writer does not know the context it's used in (nor should it). |
||
|
|
||
|
|
||
| def MakeFile(sock, mode='r', bufsize=io.DEFAULT_BUFFER_SIZE): | ||
| """File object attached to a socket object.""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Fixed socket errors when clients send HTTP to HTTPS-only ports. | ||
|
|
||
| -- by :user:`julianz-` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ netloc | |
| noqa | ||
| PIL | ||
| pipelining | ||
| plaintext | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might not need this if we don't put that comment into a docstring but have it as a comment. |
||
| positionally | ||
| pre | ||
| preconfigure | ||
|
|
||
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.
plz avoid having more than one instruction in try-blocks.
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.
Also, this should probably be a
with-block anyway.