bug #33444 [HttpClient] improve handling of HTTP/2 PUSH, disable it b…

…y default (nicolas-grekas)

This PR was merged into the 4.3 branch.

Discussion
----------

[HttpClient] improve handling of HTTP/2 PUSH, disable it by default

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This follows discussions with @dunglas
For the test cases, https://http2-push.io is down, let's use Akamai instead

This PR now considers the proxy settings before accepting a pushed response.
It also splits the responsibility of dealing with accepting pushed responses in method `acceptPushForRequest`.

The logic in this method could also be delegated to a userland callback passed as an option. Let's wait for someone with an actual use case before adding the option.

This PR also disables HTTP/2 PUSH by default because it is not stable: locally, with the latest curl version, enabling this on a server that pushes things fails with `Failure when receiving data from the peer`. This is not ready for prime time in either ext-curl or the underlying libcurl. You can still enable it explicitly by passing some positive number to the constructor.

Commits
-------

019bce7 [HttpClient] improve handling of HTTP/2 PUSH

src/Symfony/Component/HttpClient/CurlHttpClient.php

@@ -55,7 +55,7 @@ final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface
      *
      * @see HttpClientInterface::OPTIONS_DEFAULTS for available options
      */
-    public function __construct(array $defaultOptions = [], int $maxHostConnections = 6, int $maxPendingPushes = 50)
+    public function __construct(array $defaultOptions = [], int $maxHostConnections = 6, int $maxPendingPushes = 0)
     {
         if (!\extension_loaded('curl')) {
             throw new \LogicException('You cannot use the "Symfony\Component\HttpClient\CurlHttpClient" as the "curl" extension is not installed.');
@@ -105,35 +105,30 @@ public function request(string $method, string $url, array $options = []): Respo
         $host = parse_url($authority, PHP_URL_HOST);
         $url = implode('', $url);
 
+        if (!isset($options['normalized_headers']['user-agent'])) {
+            $options['normalized_headers']['user-agent'][] = 'Symfony HttpClient/Curl';
+            $options['headers'][] = 'User-Agent: Symfony HttpClient/Curl';
+        }
+
         if ($pushedResponse = $this->multi->pushedResponses[$url] ?? null) {
             unset($this->multi->pushedResponses[$url]);
-            // Accept pushed responses only if their headers related to authentication match the request
-            $expectedHeaders = ['authorization', 'cookie', 'x-requested-with', 'range'];
-            foreach ($expectedHeaders as $k => $v) {
-                $expectedHeaders[$k] = null;
-
-                foreach ($options['normalized_headers'][$v] ?? [] as $h) {
-                    $expectedHeaders[$k][] = substr($h, 2 + \strlen($v));
-                }
-            }
 
-            if ('GET' === $method && $expectedHeaders === $pushedResponse->headers && !$options['body']) {
-                $this->logger && $this->logger->debug(sprintf('Connecting request to pushed response: "%s %s"', $method, $url));
+            if (self::acceptPushForRequest($method, $options, $pushedResponse)) {
+                $this->logger && $this->logger->debug(sprintf('Accepting pushed response: "%s %s"', $method, $url));
 
                 // Reinitialize the pushed response with request's options
                 $pushedResponse->response->__construct($this->multi, $url, $options, $this->logger);
 
                 return $pushedResponse->response;
             }
 
-            $this->logger && $this->logger->debug(sprintf('Rejecting pushed response for "%s": authorization headers don\'t match the request', $url));
+            $this->logger && $this->logger->debug(sprintf('Rejecting pushed response: "%s".', $url));
         }
 
         $this->logger && $this->logger->info(sprintf('Request: "%s %s"', $method, $url));
 
         $curlopts = [
             CURLOPT_URL => $url,
-            CURLOPT_USERAGENT => 'Symfony HttpClient/Curl',
             CURLOPT_TCP_NODELAY => true,
             CURLOPT_PROTOCOLS => CURLPROTO_HTTP | CURLPROTO_HTTPS,
             CURLOPT_REDIR_PROTOCOLS => CURLPROTO_HTTP | CURLPROTO_HTTPS,
@@ -306,7 +301,7 @@ public function __destruct()
         $active = 0;
         while (CURLM_CALL_MULTI_PERFORM === curl_multi_exec($this->multi->handle, $active));
 
-        foreach ($this->multi->openHandles as $ch) {
+        foreach ($this->multi->openHandles as [$ch]) {
             curl_setopt($ch, CURLOPT_VERBOSE, false);
         }
     }
@@ -318,17 +313,17 @@ private static function handlePush($parent, $pushed, array $requestHeaders, Curl
 
         foreach ($requestHeaders as $h) {
             if (false !== $i = strpos($h, ':', 1)) {
-                $headers[substr($h, 0, $i)] = substr($h, 1 + $i);
+                $headers[substr($h, 0, $i)][] = substr($h, 1 + $i);
             }
         }
 
-        if (!isset($headers[':method']) || !isset($headers[':scheme']) || !isset($headers[':authority']) || !isset($headers[':path']) || 'GET' !== $headers[':method'] || isset($headers['range'])) {
+        if (!isset($headers[':method']) || !isset($headers[':scheme']) || !isset($headers[':authority']) || !isset($headers[':path'])) {
             $logger && $logger->debug(sprintf('Rejecting pushed response from "%s": pushed headers are invalid', $origin));
 
             return CURL_PUSH_DENY;
         }
 
-        $url = $headers[':scheme'].'://'.$headers[':authority'];
+        $url = $headers[':scheme'][0].'://'.$headers[':authority'][0];
 
         if ($maxPendingPushes <= \count($multi->pushedResponses)) {
             $logger && $logger->debug(sprintf('Rejecting pushed response from "%s" for "%s": the queue is full', $origin, $url));
@@ -345,22 +340,38 @@ private static function handlePush($parent, $pushed, array $requestHeaders, Curl
             return CURL_PUSH_DENY;
         }
 
-        $url .= $headers[':path'];
+        $url .= $headers[':path'][0];
         $logger && $logger->debug(sprintf('Queueing pushed response: "%s"', $url));
 
-        $multi->pushedResponses[$url] = new PushedResponse(
-            new CurlResponse($multi, $pushed),
-            [
-                $headers['authorization'] ?? null,
-                $headers['cookie'] ?? null,
-                $headers['x-requested-with'] ?? null,
-                null,
-            ]
-        );
+        $multi->pushedResponses[$url] = new PushedResponse(new CurlResponse($multi, $pushed), $headers, $multi->openHandles[(int) $parent][1] ?? []);
 
         return CURL_PUSH_OK;
     }
 
+    /**
+     * Accepts pushed responses only if their headers related to authentication match the request.
+     */
+    private static function acceptPushForRequest(string $method, array $options, PushedResponse $pushedResponse): bool
+    {
+        if ($options['body'] || $method !== $pushedResponse->requestHeaders[':method'][0]) {
+            return false;
+        }
+
+        foreach (['proxy', 'no_proxy', 'bindto'] as $k) {
+            if ($options[$k] !== $pushedResponse->parentOptions[$k]) {
+                return false;
+            }
+        }
+
+        foreach (['authorization', 'cookie', 'range', 'proxy-authorization'] as $k) {
+            if (($pushedResponse->requestHeaders[$k] ?? null) !== ($options['normalized_headers'][$k] ?? null)) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
     /**
      * Wraps the request's body callback to allow it to return strings longer than curl requested.
      */

src/Symfony/Component/HttpClient/Internal/PushedResponse.php

@@ -14,23 +14,25 @@
 use Symfony\Component\HttpClient\Response\CurlResponse;
 
 /**
- * A pushed response with headers.
+ * A pushed response with its request headers.
  *
  * @author Alexander M. Turek <me@derrabus.de>
  *
  * @internal
  */
 final class PushedResponse
 {
-    /** @var CurlResponse */
     public $response;
 
     /** @var string[] */
-    public $headers;
+    public $requestHeaders;
 
-    public function __construct(CurlResponse $response, array $headers)
+    public $parentOptions = [];
+
+    public function __construct(CurlResponse $response, array $requestHeaders, array $parentOptions)
     {
         $this->response = $response;
-        $this->headers = $headers;
+        $this->requestHeaders = $requestHeaders;
+        $this->parentOptions = $parentOptions;
     }
 }

src/Symfony/Component/HttpClient/Response/CurlResponse.php

@@ -120,9 +120,6 @@ public function __construct(CurlClientState $multi, $ch, array $options = null,
 
             if (\in_array($waitFor, ['headers', 'destruct'], true)) {
                 try {
-                    if (\defined('CURLOPT_STREAM_WEIGHT')) {
-                        curl_setopt($ch, CURLOPT_STREAM_WEIGHT, 32);
-                    }
                     self::stream([$response])->current();
                 } catch (\Throwable $e) {
                     // Persist timeouts thrown during initialization
@@ -140,7 +137,7 @@ public function __construct(CurlClientState $multi, $ch, array $options = null,
         };
 
         // Schedule the request in a non-blocking way
-        $multi->openHandles[$id] = $ch;
+        $multi->openHandles[$id] = [$ch, $options];
         curl_multi_add_handle($multi->handle, $ch);
         self::perform($multi);
     }

src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php

@@ -47,27 +47,22 @@ public function log($level, $message, array $context = [])
             }
         };
 
-        $client = new CurlHttpClient();
+        $client = new CurlHttpClient([], 6, 2);
         $client->setLogger($logger);
 
-        $index = $client->request('GET', 'https://http2-push.io');
+        $index = $client->request('GET', 'https://http2.akamai.com/');
         $index->getContent();
 
-        $css = $client->request('GET', 'https://http2-push.io/css/style.css');
-        $js = $client->request('GET', 'https://http2-push.io/js/http2-push.js');
+        $css = $client->request('GET', 'https://http2.akamai.com/resources/push.css');
 
         $css->getHeaders();
-        $js->getHeaders();
 
         $expected = [
-            'Request: "GET https://http2-push.io/"',
-            'Queueing pushed response: "https://http2-push.io/css/style.css"',
-            'Queueing pushed response: "https://http2-push.io/js/http2-push.js"',
-            'Response: "200 https://http2-push.io/"',
-            'Connecting request to pushed response: "GET https://http2-push.io/css/style.css"',
-            'Connecting request to pushed response: "GET https://http2-push.io/js/http2-push.js"',
-            'Response: "200 https://http2-push.io/css/style.css"',
-            'Response: "200 https://http2-push.io/js/http2-push.js"',
+            'Request: "GET https://http2.akamai.com/"',
+            'Queueing pushed response: "https://http2.akamai.com/resources/push.css"',
+            'Response: "200 https://http2.akamai.com/"',
+            'Accepting pushed response: "GET https://http2.akamai.com/resources/push.css"',
+            'Response: "200 https://http2.akamai.com/resources/push.css"',
         ];
         $this->assertSame($expected, $logger->logs);
     }