diff --git a/src/Driver/Http1Driver.php b/src/Driver/Http1Driver.php index cf6a774d..102a15b9 100644 --- a/src/Driver/Http1Driver.php +++ b/src/Driver/Http1Driver.php @@ -880,7 +880,12 @@ private function send(?Future $lastWrite, Response $response, ?Request $request $headers["trailer"] = [\implode(", ", $fields)]; } + // 204 and 304 responses are terminated by the end of the header section and have no + // body, so they must not use chunked encoding (RFC 9112 sections 6.1 and 6.3). + $bodyless = $status === HttpStatus::NO_CONTENT || $status === HttpStatus::NOT_MODIFIED; + $chunked = !$shouldClose + && !$bodyless && (!isset($headers["content-length"]) || $trailers !== null) && $protocol === "1.1" && $status >= HttpStatus::OK; @@ -898,7 +903,9 @@ private function send(?Future $lastWrite, Response $response, ?Request $request $chunk = null; // Required for the finally, not directly overwritten, even if your IDE says otherwise. - if ($request?->getMethod() === "HEAD") { + if ($bodyless || $request?->getMethod() === "HEAD") { + $need = null; // No body is written, so nothing is owed to the client. + if ($shouldClose) { $this->writableStream->end(); } diff --git a/test/Driver/Http1DriverTest.php b/test/Driver/Http1DriverTest.php index 2ee00720..7500aca5 100644 --- a/test/Driver/Http1DriverTest.php +++ b/test/Driver/Http1DriverTest.php @@ -934,6 +934,18 @@ public function provideWriteResponses(): array "HTTP/1.0 200 OK\r\nconnection: close\r\ndate: .* GMT\r\n\r\n", true, ], + [ + "GET / HTTP/1.1", + new Response(HttpStatus::NO_CONTENT, ["x-custom" => "1"], new ReadableBuffer), + "^HTTP/1.1 204 No Content\r\nx-custom: 1\r\nconnection: keep-alive\r\nkeep-alive: timeout=60\r\ndate: .* GMT\r\n\r\n$", + false, + ], + [ + "GET / HTTP/1.1", + new Response(HttpStatus::NOT_MODIFIED, ["etag" => "\"abc\""], new ReadableBuffer("ignored")), + "^HTTP/1.1 304 Not Modified\r\netag: \"abc\"\r\nconnection: keep-alive\r\nkeep-alive: timeout=60\r\ndate: .* GMT\r\n\r\n$", + false, + ], ]; delay(0.1); // Tick event loop to complete the Trailers future. @@ -941,6 +953,44 @@ public function provideWriteResponses(): array return $data; } + public function testBodylessResponseDoesNotDesyncConnection(): void + { + $driver = new Http1Driver( + new ClosureRequestHandler(function (Request $request): Response { + if ($request->getUri()->getPath() === "/first") { + return new Response(HttpStatus::NO_CONTENT, ["x-custom" => "1"]); + } + + return new Response(HttpStatus::OK, [], "second"); + }), + $this->createMock(ErrorHandler::class), + new NullLogger, + connectionTimeout: 60, + ); + + $output = new WritableBuffer; + + async(fn () => $driver->handleClient( + $this->createClientMock(), + new ReadableBuffer( + "GET /first HTTP/1.1\r\nHost: test.local\r\n\r\n" . + "GET /second HTTP/1.1\r\nHost: test.local\r\n\r\n" + ), + $output, + )); + + delay(0.1); + + $output->close(); + + // The next response must start right after the header section of the bodyless one; + // a stray chunk terminator in between desyncs clients which stop reading at the headers. + self::assertMatchesRegularExpression( + "#^HTTP/1.1 204 No Content\r\n(?:[^\r]+\r\n)+\r\nHTTP/1.1 200 OK\r\n#", + $output->buffer(), + ); + } + public function testWriteAbortAfterHeaders(): void { $driver = new Http1Driver(