From d77d8e212cde7b5c4a64142bf431522f19487c28 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Thu, 28 Nov 2024 08:55:08 +0100 Subject: [PATCH] [HttpClient] Fix streaming and redirecting with NoPrivateNetworkHttpClient --- NoPrivateNetworkHttpClient.php | 35 ++++------ .../HttpClientDataCollectorTest.php | 5 -- Tests/HttpClientTestCase.php | 67 +++++++++++++++++++ Tests/HttplugClientTest.php | 5 -- Tests/Psr18ClientTest.php | 5 -- Tests/RetryableHttpClientTest.php | 5 -- Tests/TraceableHttpClientTest.php | 5 -- 7 files changed, 81 insertions(+), 46 deletions(-) diff --git a/NoPrivateNetworkHttpClient.php b/NoPrivateNetworkHttpClient.php index 8ea8d917..ad973671 100644 --- a/NoPrivateNetworkHttpClient.php +++ b/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,24 +102,13 @@ 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']; } @@ -128,6 +116,19 @@ public function request(string $method, string $url, array $options = []): Respo 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} */ diff --git a/Tests/DataCollector/HttpClientDataCollectorTest.php b/Tests/DataCollector/HttpClientDataCollectorTest.php index 54e160b5..15a3136d 100644 --- a/Tests/DataCollector/HttpClientDataCollectorTest.php +++ b/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([ diff --git a/Tests/HttpClientTestCase.php b/Tests/HttpClientTestCase.php index 6bed6d6f..d18cc464 100644 --- a/Tests/HttpClientTestCase.php +++ b/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__); diff --git a/Tests/HttplugClientTest.php b/Tests/HttplugClientTest.php index 41ed55ed..51b469cb 100644 --- a/Tests/HttplugClientTest.php +++ b/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 */ diff --git a/Tests/Psr18ClientTest.php b/Tests/Psr18ClientTest.php index 65b7f5b3..bf49535a 100644 --- a/Tests/Psr18ClientTest.php +++ b/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 */ diff --git a/Tests/RetryableHttpClientTest.php b/Tests/RetryableHttpClientTest.php index c15b0d2a..9edf4131 100644 --- a/Tests/RetryableHttpClientTest.php +++ b/Tests/RetryableHttpClientTest.php @@ -27,11 +27,6 @@ class RetryableHttpClientTest extends TestCase { - public static function tearDownAfterClass(): void - { - TestHttpServer::stop(); - } - public function testRetryOnError() { $client = new RetryableHttpClient( diff --git a/Tests/TraceableHttpClientTest.php b/Tests/TraceableHttpClientTest.php index 052400bb..5f20e198 100644 --- a/Tests/TraceableHttpClientTest.php +++ b/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);