[HttpClient] Fix streaming and redirecting with NoPrivateNetworkHttpClient

Author: nicolas-grekas

NoPrivateNetworkHttpClient.php

@@ -20,7 +20,6 @@
 use Symfony\Contracts\HttpClient\ChunkInterface;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 use Symfony\Contracts\HttpClient\ResponseInterface;
-use Symfony\Contracts\HttpClient\ResponseStreamInterface;
 use Symfony\Contracts\Service\ResetInterface;
 
 /**
@@ -103,31 +102,33 @@ public function request(string $method, string $url, array $options = []): Respo
         $ip = self::dnsResolve($dnsCache, $host, $this->ipFlags, $options);
         self::ipCheck($ip, $this->subnets, $this->ipFlags, $host, $url);
 
-        if (0 < $maxRedirects = $options['max_redirects']) {
-            $options['max_redirects'] = 0;
-            $redirectHeaders['with_auth'] = $redirectHeaders['no_auth'] = $options['headers'];
-
-            if (isset($options['normalized_headers']['host']) || isset($options['normalized_headers']['authorization']) || isset($options['normalized_headers']['cookie'])) {
-                $redirectHeaders['no_auth'] = array_filter($redirectHeaders['no_auth'], static function ($h) {
-                    return 0 !== stripos($h, 'Host:') && 0 !== stripos($h, 'Authorization:') && 0 !== stripos($h, 'Cookie:');
-                });
-            }
-        }
-
         $onProgress = $options['on_progress'] ?? null;
         $subnets = $this->subnets;
         $ipFlags = $this->ipFlags;
         $lastPrimaryIp = '';
 
         $options['on_progress'] = static function (int $dlNow, int $dlSize, array $info) use ($onProgress, $subnets, $ipFlags, &$lastPrimaryIp): void {
-            if (($info['primary_ip'] ?? '') !== $lastPrimaryIp) {
+            if (!\in_array($info['primary_ip'] ?? '', ['', $lastPrimaryIp], true)) {
                 self::ipCheck($info['primary_ip'], $subnets, $ipFlags, null, $info['url']);
                 $lastPrimaryIp = $info['primary_ip'];
             }
 
             null !== $onProgress && $onProgress($dlNow, $dlSize, $info);
         };
 
+        if (0 >= $maxRedirects = $options['max_redirects']) {
+            return new AsyncResponse($this->client, $method, $url, $options);
+        }
+
+        $options['max_redirects'] = 0;
+        $redirectHeaders['with_auth'] = $redirectHeaders['no_auth'] = $options['headers'];
+
+        if (isset($options['normalized_headers']['host']) || isset($options['normalized_headers']['authorization']) || isset($options['normalized_headers']['cookie'])) {
+            $redirectHeaders['no_auth'] = array_filter($redirectHeaders['no_auth'], static function ($h) {
+                return 0 !== stripos($h, 'Host:') && 0 !== stripos($h, 'Authorization:') && 0 !== stripos($h, 'Cookie:');
+            });
+        }
+
         return new AsyncResponse($this->client, $method, $url, $options, static function (ChunkInterface $chunk, AsyncContext $context) use (&$method, &$options, $maxRedirects, &$redirectHeaders, $subnets, $ipFlags, $dnsCache): \Generator {
             if (null !== $chunk->getError() || $chunk->isTimeout() || !$chunk->isFirst()) {
                 yield $chunk;
@@ -178,14 +179,6 @@ public function request(string $method, string $url, array $options = []): Respo
         });
     }
 
-    /**
-     * {@inheritdoc}
-     */
-    public function stream($responses, ?float $timeout = null): ResponseStreamInterface
-    {
-        return $this->client->stream($responses, $timeout);
-    }
-
     /**
      * {@inheritdoc}
      */

Tests/DataCollector/HttpClientDataCollectorTest.php

@@ -24,11 +24,6 @@ public static function setUpBeforeClass(): void
         TestHttpServer::start();
     }
 
-    public static function tearDownAfterClass(): void
-    {
-        TestHttpServer::stop();
-    }
-
     public function testItCollectsRequestCount()
     {
         $httpClient1 = $this->httpClientThatHasTracedRequests([

Tests/HttpClientTestCase.php

@@ -523,6 +523,73 @@ public function testNoPrivateNetworkWithResolveAndRedirect()
         $client->request('GET', 'http://localhost:8057/302?location=https://symfony.com/');
     }
 
+    public function testNoPrivateNetwork304()
+    {
+        $client = $this->getHttpClient(__FUNCTION__);
+        $client = new NoPrivateNetworkHttpClient($client, '104.26.14.6/32');
+        $response = $client->request('GET', 'http://localhost:8057/304', [
+            'headers' => ['If-Match' => '"abc"'],
+            'buffer' => false,
+        ]);
+
+        $this->assertSame(304, $response->getStatusCode());
+        $this->assertSame('', $response->getContent(false));
+    }
+
+    public function testNoPrivateNetwork302()
+    {
+        $client = $this->getHttpClient(__FUNCTION__);
+        $client = new NoPrivateNetworkHttpClient($client, '104.26.14.6/32');
+        $response = $client->request('GET', 'http://localhost:8057/302/relative');
+
+        $body = $response->toArray();
+
+        $this->assertSame('/', $body['REQUEST_URI']);
+        $this->assertNull($response->getInfo('redirect_url'));
+
+        $response = $client->request('GET', 'http://localhost:8057/302/relative', [
+            'max_redirects' => 0,
+        ]);
+
+        $this->assertSame(302, $response->getStatusCode());
+        $this->assertSame('http://localhost:8057/', $response->getInfo('redirect_url'));
+    }
+
+    public function testNoPrivateNetworkStream()
+    {
+        $client = $this->getHttpClient(__FUNCTION__);
+
+        $response = $client->request('GET', 'http://localhost:8057');
+        $client = new NoPrivateNetworkHttpClient($client, '104.26.14.6/32');
+
+        $response = $client->request('GET', 'http://localhost:8057');
+        $chunks = $client->stream($response);
+        $result = [];
+
+        foreach ($chunks as $r => $chunk) {
+            if ($chunk->isTimeout()) {
+                $result[] = 't';
+            } elseif ($chunk->isLast()) {
+                $result[] = 'l';
+            } elseif ($chunk->isFirst()) {
+                $result[] = 'f';
+            }
+        }
+
+        $this->assertSame($response, $r);
+        $this->assertSame(['f', 'l'], $result);
+
+        $chunk = null;
+        $i = 0;
+
+        foreach ($client->stream($response) as $chunk) {
+            ++$i;
+        }
+
+        $this->assertSame(1, $i);
+        $this->assertTrue($chunk->isLast());
+    }
+
     public function testNoRedirectWithInvalidLocation()
     {
         $client = $this->getHttpClient(__FUNCTION__);

Tests/HttplugClientTest.php

@@ -32,11 +32,6 @@ public static function setUpBeforeClass(): void
         TestHttpServer::start();
     }
 
-    public static function tearDownAfterClass(): void
-    {
-        TestHttpServer::stop();
-    }
-
     /**
      * @requires function ob_gzhandler
      */

Tests/Psr18ClientTest.php

@@ -28,11 +28,6 @@ public static function setUpBeforeClass(): void
         TestHttpServer::start();
     }
 
-    public static function tearDownAfterClass(): void
-    {
-        TestHttpServer::stop();
-    }
-
     /**
      * @requires function ob_gzhandler
      */

Tests/RetryableHttpClientTest.php

@@ -27,11 +27,6 @@
 
 class RetryableHttpClientTest extends TestCase
 {
-    public static function tearDownAfterClass(): void
-    {
-        TestHttpServer::stop();
-    }
-
     public function testRetryOnError()
     {
         $client = new RetryableHttpClient(

Tests/TraceableHttpClientTest.php

@@ -29,11 +29,6 @@ public static function setUpBeforeClass(): void
         TestHttpServer::start();
     }
 
-    public static function tearDownAfterClass(): void
-    {
-        TestHttpServer::stop();
-    }
-
     public function testItTracesRequest()
     {
         $httpClient = $this->createMock(HttpClientInterface::class);