Unified handling of http over https errors

Added logic including a new method ( _check_for_plain_http)
in the Adapter base class to identify when a client
attempts to speak unencrypted HTTP on an HTTPS port.

When a plaintext connection is detected, the server
now attempts to access the raw underlying TCP socket (if wrapped)
to send a "400 Bad Request" error response before any TLS failure.
This unifies the behavior of the 'builtin' and 'pyopenssl'
adapters for this specific error condition.

Author: julianz-

.flake8

@@ -124,11 +124,11 @@ per-file-ignores =
   cheroot/__main__.py: WPS130
   cheroot/_compat.py: DAR101, DAR201, DAR301, DAR401, I003, RST304, WPS100, WPS111, WPS123, WPS226, WPS229, WPS420, WPS422, WPS432, WPS504, WPS505
   cheroot/cli.py: DAR101, DAR201, DAR401, I001, I004, I005, WPS100, WPS110, WPS120, WPS130, WPS202, WPS226, WPS229, WPS338, WPS420, WPS421
-  cheroot/connections.py: DAR101, DAR201, DAR301, DAR401, I001, I003, I004, I005, RST304, S104, WPS100, WPS110, WPS111, WPS121, WPS122, WPS130, WPS201, WPS204, WPS210, WPS212, WPS214, WPS220, WPS229, WPS231, WPS301, WPS324, WPS338, WPS420, WPS421, WPS422, WPS432, WPS501, WPS504, WPS505
+  cheroot/connections.py: DAR101, DAR201, DAR301, DAR401, I001, I003, I004, I005, RST304, S104, WPS100, WPS110, WPS111, WPS121, WPS122, WPS130, WPS201, WPS204, WPS210, WPS212, WPS214, WPS220, WPS229, WPS231, WPS237, WPS301, WPS324, WPS338, WPS420, WPS421, WPS422, WPS432, WPS501, WPS504, WPS505
   cheroot/errors.py: DAR101, DAR201, I003, RST304, WPS111, WPS121, WPS422
   cheroot/makefile.py: DAR101, DAR201, DAR401, E800, I003, I004, N801, N802, S101, WPS100, WPS110, WPS111, WPS117, WPS120, WPS121, WPS122, WPS123, WPS130, WPS204, WPS210, WPS212, WPS213, WPS220, WPS229, WPS231, WPS232, WPS338, WPS420, WPS422, WPS429, WPS431, WPS504, WPS604, WPS606
   cheroot/server.py: DAR003, DAR101, DAR201, DAR202, DAR301, DAR401, E800, I001, I003, I004, I005, N806, RST201, RST301, RST303, RST304, WPS100, WPS110, WPS111, WPS115, WPS120, WPS121, WPS122, WPS130, WPS132, WPS201, WPS202, WPS204, WPS210, WPS211, WPS212, WPS213, WPS214, WPS220, WPS221, WPS225, WPS226, WPS229, WPS230, WPS231, WPS236, WPS237, WPS238, WPS301, WPS338, WPS342, WPS410, WPS420, WPS421, WPS422, WPS429, WPS432, WPS504, WPS505, WPS601, WPS602, WPS608, WPS617
-  cheroot/ssl/builtin.py: DAR101, DAR201, DAR401, I001, I003, N806, RST304, WPS110, WPS111, WPS115, WPS117, WPS120, WPS121, WPS122, WPS130, WPS201, WPS210, WPS214, WPS229, WPS231, WPS338, WPS422, WPS501, WPS505, WPS529, WPS608, WPS612
+  cheroot/ssl/builtin.py: DAR101, DAR201, DAR401, I001, I003, N806, RST304, WPS110, WPS111, WPS115, WPS117, WPS120, WPS121, WPS122, WPS130, WPS201, WPS210, WPS214, WPS229, WPS231, WPS238, WPS338, WPS422, WPS501, WPS505, WPS529, WPS608, WPS612
   cheroot/ssl/pyopenssl.py: C815, DAR101, DAR201, DAR401, I001, I003, I005, N801, N804, RST304, WPS100, WPS110, WPS111, WPS117, WPS120, WPS121, WPS130, WPS210, WPS220, WPS221, WPS225, WPS229, WPS231, WPS238, WPS301, WPS335, WPS338, WPS420, WPS422, WPS430, WPS432, WPS501, WPS504, WPS505, WPS601, WPS608, WPS615
   cheroot/test/conftest.py: DAR101, DAR201, DAR301, I001, I003, I005, WPS100, WPS130, WPS325, WPS354, WPS420, WPS422, WPS430, WPS457
   cheroot/test/helper.py: DAR101, DAR201, DAR401, I001, I003, I004, N802, WPS110, WPS111, WPS121, WPS201, WPS220, WPS231, WPS301, WPS414, WPS421, WPS422, WPS505

cheroot/connections.py

@@ -1,6 +1,5 @@
 """Utilities to manage open connections."""
 
-import io
 import os
 import selectors
 import socket
@@ -306,31 +305,9 @@ def _from_server_socket(self, server_socket):  # noqa: C901  # FIXME
                         f'{tls_connection_drop_error!s}',
                     )
                     return None
-                except errors.NoSSLError as http_over_https_err:
-                    self.server.error_log(
-                        f'Client {addr!s} attempted to speak plain HTTP into '
-                        'a TCP connection configured for TLS-only traffic — '
-                        'trying to send back a plain HTTP error response: '
-                        f'{http_over_https_err!s}',
-                    )
-                    msg = (
-                        'The client sent a plain HTTP request, but '
-                        'this server only speaks HTTPS on this port.'
-                    )
-                    buf = [
-                        '%s 400 Bad Request\r\n' % self.server.protocol,
-                        'Content-Length: %s\r\n' % len(msg),
-                        'Content-Type: text/plain\r\n\r\n',
-                        msg,
-                    ]
-
-                    wfile = mf(s, 'wb', io.DEFAULT_BUFFER_SIZE)
-                    try:
-                        wfile.write(''.join(buf).encode('ISO-8859-1'))
-                    except OSError as ex:
-                        if ex.args[0] not in errors.socket_errors_to_ignore:
-                            raise
-                    return None
+                except errors.NoSSLError:
+                    return self._send_bad_request_plain_http_error(s, addr)
+
                 mf = self.server.ssl_adapter.makefile
                 # Re-apply our timeout since we may have a new socket object
                 if hasattr(s, 'settimeout'):
@@ -403,3 +380,50 @@ def can_add_keepalive_connection(self):
         """Flag whether it is allowed to add a new keep-alive connection."""
         ka_limit = self.server.keep_alive_conn_limit
         return ka_limit is None or self._num_connections < ka_limit
+
+    def _send_bad_request_plain_http_error(self, sock, addr):
+        """Send Bad Request 400 response, and close the socket."""
+        self.server.error_log(
+            f'Client {addr!s} attempted to speak plain HTTP into '
+            'a TCP connection configured for TLS-only traffic — '
+            'Sending 400 Bad Request.',
+        )
+
+        msg = (
+            'The client sent a plain HTTP request, but this server '
+            'only speaks HTTPS on this port.'
+        )
+
+        response_parts = [
+            f'{self.server.protocol} 400 Bad Request\r\n',
+            'Content-Type: text/plain\r\n',
+            f'Content-Length: {len(msg)}\r\n',
+            'Connection: close\r\n',
+            '\r\n',
+            msg,
+        ]
+        response_bytes = ''.join(response_parts).encode('ISO-8859-1')
+
+        # The original socket (sock) is SSL-wrapped, but we must send the 400
+        # response unencrypted over the raw TCP channel.
+        sock_to_send = sock
+        # Handle both raw sockets and SSL connections
+        if hasattr(sock, 'do_handshake'):
+            with suppress(AttributeError):
+                # Access the raw, underlying TCP socket via the
+                # common '._socket' attribute to bypass the SSL layer.
+                # fall back to using the given socket directly if
+                # the attribute does not exist.
+                sock_to_send = sock._socket
+        try:
+            # Use the determined socket (raw TCP if successfully retrieved)
+            sock_to_send.sendall(response_bytes)
+
+            # Shutdown the writing end of the socket used for sending.
+            sock_to_send.shutdown(socket.SHUT_WR)
+
+        except OSError as ex:
+            if ex.args[0] not in errors.socket_errors_to_ignore:
+                raise
+
+        sock.close()

cheroot/ssl/__init__.py

@@ -1,5 +1,6 @@
 """Implementation of the SSL adapter base interface."""
 
+import socket
 from abc import ABC, abstractmethod
 
 
@@ -50,3 +51,43 @@ def get_environ(self):
     def makefile(self, sock, mode='r', bufsize=-1):
         """Return socket file object."""
         raise NotImplementedError  # pragma: no cover
+
+    def _check_for_plain_http(self, raw_socket):
+        """Check if the client sent plain HTTP by peeking at first bytes.
+
+        This is a best-effort check to provide a helpful error message when
+        clients accidentally use HTTP on an HTTPS port. If we can't detect
+        plain HTTP (timeout, no data yet, etc), we return False and let the
+        SSL handshake proceed, which will fail with its own error.
+
+        Returns:
+            bool: True if plain HTTP is detected, False otherwise
+        """
+        PEEK_BYTES = 16
+        PEEK_TIMEOUT = 0.5
+
+        original_timeout = raw_socket.gettimeout()
+        raw_socket.settimeout(PEEK_TIMEOUT)
+
+        try:
+            first_bytes = raw_socket.recv(PEEK_BYTES, socket.MSG_PEEK)
+        except (OSError, socket.timeout):
+            return False
+        finally:
+            raw_socket.settimeout(original_timeout)
+
+        if not first_bytes:
+            return False
+
+        http_methods = (
+            b'GET ',
+            b'POST ',
+            b'PUT ',
+            b'DELETE ',
+            b'HEAD ',
+            b'OPTIONS ',
+            b'PATCH ',
+            b'CONNECT ',
+            b'TRACE ',
+        )
+        return first_bytes.startswith(http_methods)

cheroot/ssl/builtin.py

@@ -309,6 +309,9 @@ def bind(self, sock):
 
     def wrap(self, sock):
         """Wrap and return the given socket, plus WSGI environ entries."""
+        if self._check_for_plain_http(sock):
+            raise errors.NoSSLError
+
         try:
             s = self.context.wrap_socket(
                 sock,

cheroot/ssl/pyopenssl.py

@@ -333,6 +333,15 @@ def wrap(self, sock):
         # pyOpenSSL doesn't perform the handshake until the first read/write
         # forcing the handshake to complete tends to result in the connection
         # closing so we can't reliably access protocol/client cert for the env
+
+        # Get raw socket for plain HTTP check
+        if isinstance(sock, ssl_conn_type):
+            raw_sock = sock._socket
+        else:
+            raw_sock = sock
+
+        if self._check_for_plain_http(raw_sock):
+            raise errors.NoSSLError
         return sock, self._environ.copy()
 
     def _password_callback(

cheroot/test/test_ssl.py

@@ -32,9 +32,7 @@
     IS_LINUX,
     IS_MACOS,
     IS_PYPY,
-    IS_SOLARIS,
     IS_WINDOWS,
-    SYS_PLATFORM,
     bton,
     ntob,
     ntou,
@@ -304,7 +302,6 @@ def test_ssl_adapters(  # pylint: disable=too-many-positional-arguments
         timeout=http_request_timeout,
         verify=tls_ca_certificate_pem_path,
     )
-
     assert resp.status_code == 200
     assert resp.text == 'Hello world!'
 
@@ -710,7 +707,6 @@ def test_https_over_http_error(http_server, ip_addr):
         pytest.param(ANY_INTERFACE_IPV6, marks=missing_ipv6),
     ),
 )
-@pytest.mark.flaky(reruns=3, reruns_delay=2)
 # pylint: disable-next=too-many-positional-arguments
 def test_http_over_https_error(
     http_request_timeout,
@@ -723,36 +719,6 @@ def test_http_over_https_error(
     tls_certificate_private_key_pem_path,
 ):
     """Ensure that connecting over HTTP to HTTPS port is handled."""
-    # disable some flaky tests
-    # https://github.com/cherrypy/cheroot/issues/225
-    issue_225 = IS_MACOS and adapter_type == 'builtin'
-    if issue_225:
-        pytest.xfail('Test fails in Travis-CI')
-
-    if IS_LINUX:
-        expected_error_code, expected_error_text = (
-            104,
-            'Connection reset by peer',
-        )
-    elif IS_MACOS:
-        expected_error_code, expected_error_text = (
-            54,
-            'Connection reset by peer',
-        )
-    elif IS_SOLARIS:
-        expected_error_code, expected_error_text = (
-            None,
-            'Remote end closed connection without response',
-        )
-    elif IS_WINDOWS:
-        expected_error_code, expected_error_text = (
-            10054,
-            'An existing connection was forcibly closed by the remote host',
-        )
-    else:
-        expected_error_code, expected_error_text = None, None
-        pytest.skip(f'{SYS_PLATFORM} is unsupported')  # pragma: no cover
-
     tls_adapter_cls = get_ssl_adapter_class(name=adapter_type)
     tls_adapter = tls_adapter_cls(
         tls_certificate_chain_pem_path,
@@ -774,31 +740,15 @@ def test_http_over_https_error(
     if ip_addr is ANY_INTERFACE_IPV6:
         fqdn = '[{fqdn}]'.format(**locals())
 
-    expect_fallback_response_over_plain_http = adapter_type == 'pyopenssl'
-    if expect_fallback_response_over_plain_http:
-        resp = requests.get(
-            f'http://{fqdn!s}:{port!s}/',
-            timeout=http_request_timeout,
-        )
-        assert resp.status_code == 400
-        assert resp.text == (
-            'The client sent a plain HTTP request, '
-            'but this server only speaks HTTPS on this port.'
-        )
-        return
-
-    with pytest.raises(requests.exceptions.ConnectionError) as ssl_err:
-        requests.get(  # FIXME: make stdlib ssl behave like PyOpenSSL
-            f'http://{fqdn!s}:{port!s}/',
-            timeout=http_request_timeout,
-        )
-
-    underlying_error = ssl_err.value.args[0].args[-1]
-    err_text = str(underlying_error)
-    assert underlying_error.errno == expected_error_code, (
-        'The underlying error is {underlying_error!r}'.format(**locals())
+    resp = requests.get(
+        f'http://{fqdn!s}:{port!s}/',
+        timeout=http_request_timeout,
+    )
+    assert resp.status_code == 400
+    assert resp.text == (
+        'The client sent a plain HTTP request, '
+        'but this server only speaks HTTPS on this port.'
     )
-    assert expected_error_text in err_text
 
 
 @pytest.mark.parametrize('adapter_type', ('builtin', 'pyopenssl'))

docs/changelog-fragments.d/800.contrib.rst

@@ -0,0 +1,3 @@
+Added unified handling of HTTP over HTTPS errors.
+
+-- by :user:`julianz-`