🐛 fix http2 maximum frame size error when the remote explicitly set a…

… lower value than the default blocksize (#144)

…
Originally found at
internetstandards/Internet.nl#1485

.github/workflows/ci.yml

@@ -45,7 +45,7 @@ jobs:
         python-version: ["3.7", "3.8", "3.9", "3.10", "3.11", "3.12", "3.13"]
         os:
           - macos-12
-          - windows-2019
+          - windows-2022
           - ubuntu-20.04  # OpenSSL 1.1.1
           - ubuntu-latest  # OpenSSL 3.0
         nox-session: ['']
@@ -89,7 +89,7 @@ jobs:
             os: ubuntu-22.04
 
     runs-on: ${{ matrix.os }}
-    name: ${{ fromJson('{"macos-12":"macOS","windows-2019":"Windows","ubuntu-latest":"Ubuntu","ubuntu-20.04":"Ubuntu 20.04 (OpenSSL 1.1.1)","ubuntu-latest":"Ubuntu Latest (OpenSSL 3+)"}')[matrix.os] }} ${{ matrix.python-version }} ${{ matrix.nox-session }}
+    name: ${{ fromJson('{"macos-12":"macOS","windows-2022":"Windows","ubuntu-latest":"Ubuntu","ubuntu-20.04":"Ubuntu 20.04 (OpenSSL 1.1.1)","ubuntu-latest":"Ubuntu Latest (OpenSSL 3+)"}')[matrix.os] }} ${{ matrix.python-version }} ${{ matrix.nox-session }}
     continue-on-error: ${{ matrix.experimental }}
     timeout-minutes: 40
     steps:

CHANGES.rst

@@ -1,3 +1,10 @@
+2.8.907 (2024-08-20)
+====================
+
+- Fixed http2 maximum frame size error when the remote explicitly set a lower value than the default blocksize.
+  This can happen when facing old server like Apache < 2.5 see https://github.com/apache/httpd/commit/ff6b8026acb8610e4faf10ee345141a3da85946e
+  Now we monitor the max_frame setting value to ensure we don't exceed it.
+
 2.8.906 (2024-08-15)
 ====================
 

docker-compose.win.yaml

@@ -1,6 +1,6 @@
 services:
   proxy:
-    image: traefik:v2.10.4-windowsservercore-1809
+    image: traefik:v3.1-windowsservercore-ltsc2022
     restart: unless-stopped
     healthcheck:
       test: [ "CMD", "traefik" ,"healthcheck", "--ping" ]
@@ -51,7 +51,6 @@ services:
       - --entrypoints.alt-http.address=:9999
       - --entrypoints.alt-https.address=:8754
       # QUIC Related Configuration
-      - --experimental.http3=true
       - --entrypoints.https.http3=true
       - --entrypoints.alt-https.http3=false
       # Enable the access log, with HTTP requests

docker-compose.yaml

@@ -1,6 +1,6 @@
 services:
   proxy:
-    image: traefik:v2.10.4
+    image: traefik:v3.1
     restart: unless-stopped
     healthcheck:
       test: [ "CMD", "traefik" ,"healthcheck", "--ping" ]
@@ -48,7 +48,6 @@ services:
       - --entrypoints.alt-http.address=:9999
       - --entrypoints.alt-https.address=:8754
       # QUIC Related Configuration
-      - --experimental.http3=true
       - --entrypoints.https.http3=true
       - --entrypoints.alt-https.http3=false
       # Enable the access log, with HTTP requests

src/urllib3/_async/connection.py

@@ -372,7 +372,7 @@ async def request(
         chunks_and_cl = body_to_chunks(
             body,
             method=method,
-            blocksize=self.blocksize,
+            blocksize=self.max_frame_size,
             force=self._svn != HttpVersion.h11,
         )
         is_sending_string = chunks_and_cl.is_string

src/urllib3/_async/connectionpool.py

@@ -1087,6 +1087,10 @@ async def _make_request(
                 )
             conn.timeout = read_timeout
 
+        http_vsn_str = (
+            conn._http_vsn_str
+        )  # keep vsn here, as conn may be upgraded afterward.
+
         # Receive the response from the server
         try:
             response = await conn.getresponse(police_officer=self.pool)
@@ -1106,7 +1110,7 @@ async def _make_request(
             method,
             url,
             # HTTP version
-            conn._http_vsn_str,
+            http_vsn_str,
             response.status,
             response.length_remaining,
         )

src/urllib3/_version.py

@@ -1,4 +1,4 @@
 # This file is protected via CODEOWNERS
 from __future__ import annotations
 
-__version__ = "2.8.906"
+__version__ = "2.8.907"

src/urllib3/backend/_async/hface.py

@@ -122,6 +122,23 @@ def is_saturated(self) -> bool:
     def is_multiplexed(self) -> bool:
         return self._protocol is not None and self._protocol.multiplexed
 
+    @property
+    def max_frame_size(self) -> int:
+        """
+        The remote may require us to a lower blocksize
+        than defined. This property will avoid relying
+        too much on defined blocksize.
+        """
+        if self._protocol is None:
+            return self.blocksize
+
+        try:
+            remote_max_size = self._protocol.max_frame_size()
+        except NotImplementedError:
+            return self.blocksize
+
+        return remote_max_size if self.blocksize > remote_max_size else self.blocksize
+
     async def _new_conn(self) -> AsyncSocket | None:  # type: ignore[override]
         # handle if set up, quic cache capability. thus avoiding first TCP request prior to upgrade.
         if (

src/urllib3/backend/_base.py

@@ -429,6 +429,10 @@ def is_idle(self) -> bool:
     def is_multiplexed(self) -> bool:
         raise NotImplementedError
 
+    @property
+    def max_frame_size(self) -> int:
+        raise NotImplementedError
+
     def _upgrade(self) -> None:
         """Upgrade conn from svn ver to max supported."""
         raise NotImplementedError

src/urllib3/backend/hface.py

@@ -136,6 +136,18 @@ def is_saturated(self) -> bool:
     def is_multiplexed(self) -> bool:
         return self._protocol is not None and self._protocol.multiplexed
 
+    @property
+    def max_frame_size(self) -> int:
+        if self._protocol is None:
+            return self.blocksize
+
+        try:
+            remote_max_size = self._protocol.max_frame_size()
+        except NotImplementedError:
+            return self.blocksize
+
+        return remote_max_size if self.blocksize > remote_max_size else self.blocksize
+
     def _new_conn(self) -> socket.socket | None:
         # handle if set up, quic cache capability. thus avoiding first TCP request prior to upgrade.
         if (

src/urllib3/connection.py

@@ -379,7 +379,7 @@ def request(
         chunks_and_cl = body_to_chunks(
             body,
             method=method,
-            blocksize=self.blocksize,
+            blocksize=self.max_frame_size,
             force=self._svn != HttpVersion.h11,
         )
         is_sending_string = chunks_and_cl.is_string

src/urllib3/connectionpool.py

@@ -1065,6 +1065,10 @@ def _make_request(
             conn.timeout = read_timeout
 
         # Receive the response from the server
+        http_vsn_str = (
+            conn._http_vsn_str
+        )  # keep vsn here, as conn may be upgraded afterward.
+
         try:
             response = conn.getresponse(police_officer=self.pool)
         except (BaseSSLError, OSError) as e:
@@ -1083,7 +1087,7 @@ def _make_request(
             method,
             url,
             # HTTP version
-            conn._http_vsn_str,
+            http_vsn_str,
             response.status,
             response.length_remaining,
         )

src/urllib3/contrib/hface/protocols/_protocols.py

@@ -63,6 +63,12 @@ def should_wait_remote_flow_control(
         """
         raise NotImplementedError
 
+    def max_frame_size(self) -> int:
+        """
+        Determine if the remote set a limited size for each data frame.
+        """
+        raise NotImplementedError
+
 
 class OverTCPProtocol(BaseProtocol, metaclass=ABCMeta):
     """

src/urllib3/contrib/hface/protocols/http2/_h2.py

@@ -85,6 +85,10 @@ def __init__(
         self._terminated: bool = False
         self._goaway_to_honor: bool = False
         self._max_stream_count = self._connection.remote_settings.max_concurrent_streams
+        self._max_frame_size = self._connection.remote_settings.max_frame_size
+
+    def max_frame_size(self) -> int:
+        return self._max_frame_size  # type: ignore[no-any-return]
 
     @staticmethod
     def exceptions() -> tuple[type[BaseException], ...]:
@@ -155,8 +159,10 @@ def _map_events(self, h2_events: list[jh2.events.Event]) -> Iterator[Event]:
                 yield DataReceived(e.stream_id, e.data, end_stream=end_stream)
             elif isinstance(e, jh2.events.StreamReset):
                 self._open_stream_count -= 1
-                stream = self._connection.streams.pop(e.stream_id)
-                self._connection._closed_streams[e.stream_id] = stream.closed_by
+                # event StreamEnded may occur before StreamReset
+                if e.stream_id in self._connection.streams:
+                    stream = self._connection.streams.pop(e.stream_id)
+                    self._connection._closed_streams[e.stream_id] = stream.closed_by
                 yield StreamResetReceived(e.stream_id, e.error_code)
             elif isinstance(e, jh2.events.ConnectionTerminated):
                 # ConnectionTerminated from h2 means that GOAWAY was received.
@@ -202,6 +208,7 @@ def bytes_received(self, data: bytes) -> None:
             self._max_stream_count = (
                 self._connection.remote_settings.max_concurrent_streams
             )
+            self._max_frame_size = self._connection.remote_settings.max_frame_size
 
     def bytes_to_send(self) -> bytes:
         return self._connection.data_to_send()  # type: ignore[no-any-return]

traefik/patched.Dockerfile

@@ -7,7 +7,7 @@ COPY . .
 RUN mkdir dist
 RUN go build -ldflags="-s" -o dist/go-httpbin.exe ./cmd/go-httpbin
 
-FROM mcr.microsoft.com/windows/nanoserver:ltsc2019
+FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
 
 COPY --from=build /go/src/github.com/mccutchen/go-httpbin/dist /app