Skip to content

Commit 5cbdb87

Browse files
nicolas-grekasnicolas-grekas
committed
minor symfony#58969 [HttpClient] Various cleanups after recent changes (nicolas-grekas)
This PR was merged into the 5.4 branch. Discussion ---------- [HttpClient] Various cleanups after recent changes | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT Recent changes motivated by security issues made me deep dive into the code of HttpClient again. Here are some cleanups I'd like to do on branch 5.4 Commits ------- 01153f2 [HttpClient] Various cleanups after recent changes
2 parents 8840b71 + 01153f2 commit 5cbdb87

File tree

10 files changed

+48
-84
lines changed

10 files changed

+48
-84
lines changed

‎src/Symfony/Component/HttpClient/CurlHttpClient.php‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -421,9 +421,8 @@ private static function createRedirectResolver(array $options, string $host): \C
421421
}
422422
}
423423

424-
return static function ($ch, string $location, bool $noContent, bool &$locationHasHost) use (&$redirectHeaders, $options) {
424+
return static function ($ch, string $location, bool $noContent) use (&$redirectHeaders, $options) {
425425
try {
426-
$locationHasHost = false;
427426
$location = self::parseUrl($location);
428427
$url = self::parseUrl(curl_getinfo($ch, \CURLINFO_EFFECTIVE_URL));
429428
$url = self::resolveUrl($location, $url);
@@ -439,9 +438,7 @@ private static function createRedirectResolver(array $options, string $host): \C
439438
$redirectHeaders['with_auth'] = array_filter($redirectHeaders['with_auth'], $filterContentHeaders);
440439
}
441440

442-
$locationHasHost = isset($location['authority']);
443-
444-
if ($redirectHeaders && $locationHasHost) {
441+
if ($redirectHeaders && isset($location['authority'])) {
445442
$requestHeaders = parse_url($location['authority'], \PHP_URL_HOST) === $redirectHeaders['host'] ? $redirectHeaders['with_auth'] : $redirectHeaders['no_auth'];
446443
curl_setopt($ch, \CURLOPT_HTTPHEADER, $requestHeaders);
447444
} elseif ($noContent && $redirectHeaders) {

‎src/Symfony/Component/HttpClient/NativeHttpClient.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ public function request(string $method, string $url, array $options = []): Respo
156156

157157
$progressInfo = $info;
158158
$progressInfo['url'] = implode('', $info['url']);
159+
$progressInfo['resolve'] = $resolve;
159160
unset($progressInfo['size_body']);
160161

161162
if ($progress && -1 === $progress[0]) {
@@ -165,7 +166,7 @@ public function request(string $method, string $url, array $options = []): Respo
165166
$lastProgress = $progress ?: $lastProgress;
166167
}
167168

168-
$onProgress($lastProgress[0], $lastProgress[1], $progressInfo, $resolve);
169+
$onProgress($lastProgress[0], $lastProgress[1], $progressInfo);
169170
};
170171
} elseif (0 < $options['max_duration']) {
171172
$maxDuration = $options['max_duration'];
@@ -348,6 +349,7 @@ private static function dnsResolve($host, NativeClientState $multi, array &$info
348349

349350
$multi->dnsCache[$host] = $ip = $ip[0];
350351
$info['debug'] .= "* Added {$host}:0:{$ip} to DNS cache\n";
352+
$host = $ip;
351353
} else {
352354
$info['debug'] .= "* Hostname was found in DNS cache\n";
353355
$host = str_contains($ip, ':') ? "[$ip]" : $ip;

‎src/Symfony/Component/HttpClient/NoPrivateNetworkHttpClient.php‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -80,24 +80,17 @@ public function request(string $method, string $url, array $options = []): Respo
8080
$lastUrl = '';
8181
$lastPrimaryIp = '';
8282

83-
$options['on_progress'] = function (int $dlNow, int $dlSize, array $info, ?\Closure $resolve = null) use ($onProgress, $subnets, &$lastUrl, &$lastPrimaryIp): void {
83+
$options['on_progress'] = function (int $dlNow, int $dlSize, array $info) use ($onProgress, $subnets, &$lastUrl, &$lastPrimaryIp): void {
8484
if ($info['url'] !== $lastUrl) {
85-
$host = trim(parse_url($info['url'], PHP_URL_HOST) ?: '', '[]');
85+
$host = parse_url($info['url'], PHP_URL_HOST) ?: '';
86+
$resolve = $info['resolve'] ?? static function () { return null; };
8687

87-
if (null === $resolve) {
88-
$resolve = static function () { return null; };
89-
}
90-
91-
if (($ip = $host)
92-
&& !filter_var($ip, \FILTER_VALIDATE_IP, \FILTER_FLAG_IPV6)
93-
&& !filter_var($ip, \FILTER_VALIDATE_IP, \FILTER_FLAG_IPV4)
94-
&& !$ip = $resolve($host)
88+
if (($ip = trim($host, '[]'))
89+
&& !filter_var($ip, \FILTER_VALIDATE_IP)
90+
&& !($ip = $resolve($host))
91+
&& $ip = @(gethostbynamel($host)[0] ?? dns_get_record($host, \DNS_AAAA)[0]['ipv6'] ?? null)
9592
) {
96-
if ($ip = @(dns_get_record($host, \DNS_A)[0]['ip'] ?? null)) {
97-
$resolve($host, $ip);
98-
} elseif ($ip = @(dns_get_record($host, \DNS_AAAA)[0]['ipv6'] ?? null)) {
99-
$resolve($host, '['.$ip.']');
100-
}
93+
$resolve($host, $ip);
10194
}
10295

10396
if ($ip && IpUtils::checkIp($ip, $subnets ?? self::PRIVATE_SUBNETS)) {

‎src/Symfony/Component/HttpClient/Response/AmpResponse.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,8 @@ public function __construct(AmpClientState $multi, Request $request, array $opti
9999
$onProgress = $options['on_progress'] ?? static function () {};
100100
$onProgress = $this->onProgress = static function () use (&$info, $onProgress, $resolve) {
101101
$info['total_time'] = microtime(true) - $info['start_time'];
102-
$onProgress((int) $info['size_download'], ((int) (1 + $info['download_content_length']) ?: 1) - 1, (array) $info, $resolve);
102+
$info['resolve'] = $resolve;
103+
$onProgress((int) $info['size_download'], ((int) (1 + $info['download_content_length']) ?: 1) - 1, (array) $info);
103104
};
104105

105106
$pauseDeferred = new Deferred();

‎src/Symfony/Component/HttpClient/Response/AsyncContext.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,8 +156,8 @@ public function replaceRequest(string $method, string $url, array $options = [])
156156
$this->info['previous_info'][] = $info = $this->response->getInfo();
157157
if (null !== $onProgress = $options['on_progress'] ?? null) {
158158
$thisInfo = &$this->info;
159-
$options['on_progress'] = static function (int $dlNow, int $dlSize, array $info, ?\Closure $resolve = null) use (&$thisInfo, $onProgress) {
160-
$onProgress($dlNow, $dlSize, $thisInfo + $info, $resolve);
159+
$options['on_progress'] = static function (int $dlNow, int $dlSize, array $info) use (&$thisInfo, $onProgress) {
160+
$onProgress($dlNow, $dlSize, $thisInfo + $info);
161161
};
162162
}
163163
if (0 < ($info['max_duration'] ?? 0) && 0 < ($info['total_time'] ?? 0)) {

‎src/Symfony/Component/HttpClient/Response/AsyncResponse.php‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,8 @@ public function __construct(HttpClientInterface $client, string $method, string
5151

5252
if (null !== $onProgress = $options['on_progress'] ?? null) {
5353
$thisInfo = &$this->info;
54-
$options['on_progress'] = static function (int $dlNow, int $dlSize, array $info, ?\Closure $resolve = null) use (&$thisInfo, $onProgress) {
55-
$onProgress($dlNow, $dlSize, $thisInfo + $info, $resolve);
54+
$options['on_progress'] = static function (int $dlNow, int $dlSize, array $info) use (&$thisInfo, $onProgress) {
55+
$onProgress($dlNow, $dlSize, $thisInfo + $info);
5656
};
5757
}
5858
$this->response = $client->request($method, $url, ['buffer' => false] + $options);
@@ -117,11 +117,20 @@ public function getHeaders(bool $throw = true): array
117117

118118
public function getInfo(?string $type = null)
119119
{
120+
if ('debug' === ($type ?? 'debug')) {
121+
$debug = implode('', array_column($this->info['previous_info'] ?? [], 'debug'));
122+
$debug .= $this->response->getInfo('debug');
123+
124+
if ('debug' === $type) {
125+
return $debug;
126+
}
127+
}
128+
120129
if (null !== $type) {
121130
return $this->info[$type] ?? $this->response->getInfo($type);
122131
}
123132

124-
return $this->info + $this->response->getInfo();
133+
return array_merge($this->info + $this->response->getInfo(), ['debug' => $debug]);
125134
}
126135

127136
public function toStream(bool $throw = true)
@@ -249,6 +258,7 @@ public static function stream(iterable $responses, ?float $timeout = null, ?stri
249258
return;
250259
}
251260

261+
$chunk = null;
252262
foreach ($client->stream($wrappedResponses, $timeout) as $response => $chunk) {
253263
$r = $asyncMap[$response];
254264

@@ -291,6 +301,9 @@ public static function stream(iterable $responses, ?float $timeout = null, ?stri
291301
}
292302
}
293303

304+
if (null === $chunk) {
305+
throw new \LogicException(\sprintf('"%s" is not compliant with HttpClientInterface: its "stream()" method didn\'t yield any chunks when it should have.', get_debug_type($client)));
306+
}
294307
if (null === $chunk->getError() && $chunk->isLast()) {
295308
$r->yieldedState = self::LAST_CHUNK_YIELDED;
296309
}

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

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ public function __construct(CurlClientState $multi, $ch, ?array $options = null,
128128
try {
129129
rewind($debugBuffer);
130130
$debug = ['debug' => stream_get_contents($debugBuffer)];
131-
$onProgress($dlNow, $dlSize, $url + curl_getinfo($ch) + $info + $debug, $resolve);
131+
$onProgress($dlNow, $dlSize, $url + curl_getinfo($ch) + $info + $debug + ['resolve' => $resolve]);
132132
} catch (\Throwable $e) {
133133
$multi->handlesActivity[(int) $ch][] = null;
134134
$multi->handlesActivity[(int) $ch][] = $e;
@@ -436,21 +436,11 @@ private static function parseHeaderLine($ch, string $data, array &$info, array &
436436
$info['http_method'] = 'HEAD' === $info['http_method'] ? 'HEAD' : 'GET';
437437
curl_setopt($ch, \CURLOPT_CUSTOMREQUEST, $info['http_method']);
438438
}
439-
$locationHasHost = false;
440439

441-
if (null === $info['redirect_url'] = $resolveRedirect($ch, $location, $noContent, $locationHasHost)) {
440+
if (null === $info['redirect_url'] = $resolveRedirect($ch, $location, $noContent)) {
442441
$options['max_redirects'] = curl_getinfo($ch, \CURLINFO_REDIRECT_COUNT);
443442
curl_setopt($ch, \CURLOPT_FOLLOWLOCATION, false);
444443
curl_setopt($ch, \CURLOPT_MAXREDIRS, $options['max_redirects']);
445-
} elseif ($locationHasHost) {
446-
$url = parse_url($info['redirect_url']);
447-
448-
if (null !== $ip = $multi->dnsCache->hostnames[$url['host'] = strtolower($url['host'])] ?? null) {
449-
// Populate DNS cache for redirects if needed
450-
$port = $url['port'] ?? ('http' === $url['scheme'] ? 80 : 443);
451-
curl_setopt($ch, \CURLOPT_RESOLVE, ["{$url['host']}:$port:$ip"]);
452-
$multi->dnsCache->removals["-{$url['host']}:$port"] = "-{$url['host']}:$port";
453-
}
454444
}
455445
}
456446

‎src/Symfony/Component/HttpClient/Tests/NoPrivateNetworkHttpClientTest.php‎

Lines changed: 8 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public function testExcludeByIp(string $ipAddr, $subnets, bool $mustThrow)
7575
$this->expectExceptionMessage(sprintf('IP "%s" is blocked for "%s".', $ipAddr, $url));
7676
}
7777

78-
$previousHttpClient = $this->getHttpClientMock($url, $ipAddr, $content);
78+
$previousHttpClient = $this->getMockHttpClient($ipAddr, $content);
7979
$client = new NoPrivateNetworkHttpClient($previousHttpClient, $subnets);
8080
$response = $client->request('GET', $url);
8181

@@ -91,14 +91,15 @@ public function testExcludeByIp(string $ipAddr, $subnets, bool $mustThrow)
9191
public function testExcludeByHost(string $ipAddr, $subnets, bool $mustThrow)
9292
{
9393
$content = 'foo';
94-
$url = sprintf('http://%s/', str_contains($ipAddr, ':') ? sprintf('[%s]', $ipAddr) : $ipAddr);
94+
$host = str_contains($ipAddr, ':') ? sprintf('[%s]', $ipAddr) : $ipAddr;
95+
$url = sprintf('http://%s/', $host);
9596

9697
if ($mustThrow) {
9798
$this->expectException(TransportException::class);
98-
$this->expectExceptionMessage(sprintf('Host "%s" is blocked for "%s".', $ipAddr, $url));
99+
$this->expectExceptionMessage(sprintf('Host "%s" is blocked for "%s".', $host, $url));
99100
}
100101

101-
$previousHttpClient = $this->getHttpClientMock($url, $ipAddr, $content);
102+
$previousHttpClient = $this->getMockHttpClient($ipAddr, $content);
102103
$client = new NoPrivateNetworkHttpClient($previousHttpClient, $subnets);
103104
$response = $client->request('GET', $url);
104105

@@ -119,7 +120,7 @@ public function testCustomOnProgressCallback()
119120
++$executionCount;
120121
};
121122

122-
$previousHttpClient = $this->getHttpClientMock($url, $ipAddr, $content);
123+
$previousHttpClient = $this->getMockHttpClient($ipAddr, $content);
123124
$client = new NoPrivateNetworkHttpClient($previousHttpClient);
124125
$response = $client->request('GET', $url, ['on_progress' => $customCallback]);
125126

@@ -132,7 +133,6 @@ public function testNonCallableOnProgressCallback()
132133
{
133134
$ipAddr = '104.26.14.6';
134135
$url = sprintf('http://%s/', $ipAddr);
135-
$content = 'bar';
136136
$customCallback = sprintf('cb_%s', microtime(true));
137137

138138
$this->expectException(InvalidArgumentException::class);
@@ -150,38 +150,8 @@ public function testConstructor()
150150
new NoPrivateNetworkHttpClient(new MockHttpClient(), 3);
151151
}
152152

153-
private function getHttpClientMock(string $url, string $ipAddr, string $content)
153+
private function getMockHttpClient(string $ipAddr, string $content)
154154
{
155-
$previousHttpClient = $this
156-
->getMockBuilder(HttpClientInterface::class)
157-
->getMock();
158-
159-
$previousHttpClient
160-
->expects($this->once())
161-
->method('request')
162-
->with(
163-
'GET',
164-
$url,
165-
$this->callback(function ($options) {
166-
$this->assertArrayHasKey('on_progress', $options);
167-
$onProgress = $options['on_progress'];
168-
$this->assertIsCallable($onProgress);
169-
170-
return true;
171-
})
172-
)
173-
->willReturnCallback(function ($method, $url, $options) use ($ipAddr, $content): ResponseInterface {
174-
$info = [
175-
'primary_ip' => $ipAddr,
176-
'url' => $url,
177-
];
178-
179-
$onProgress = $options['on_progress'];
180-
$onProgress(0, 0, $info);
181-
182-
return MockResponse::fromRequest($method, $url, [], new MockResponse($content));
183-
});
184-
185-
return $previousHttpClient;
155+
return new MockHttpClient(new MockResponse($content, ['primary_ip' => $ipAddr]));
186156
}
187157
}

‎src/Symfony/Component/HttpClient/TraceableHttpClient.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,11 @@ public function request(string $method, string $url, array $options = []): Respo
5858
$content = false;
5959
}
6060

61-
$options['on_progress'] = function (int $dlNow, int $dlSize, array $info, ?\Closure $resolve = null) use (&$traceInfo, $onProgress) {
61+
$options['on_progress'] = function (int $dlNow, int $dlSize, array $info) use (&$traceInfo, $onProgress) {
6262
$traceInfo = $info;
6363

6464
if (null !== $onProgress) {
65-
$onProgress($dlNow, $dlSize, $info, $resolve);
65+
$onProgress($dlNow, $dlSize, $info);
6666
}
6767
};
6868

‎src/Symfony/Contracts/HttpClient/HttpClientInterface.php‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,9 @@ interface HttpClientInterface
4848
'buffer' => true, // bool|resource|\Closure - whether the content of the response should be buffered or not,
4949
// or a stream resource where the response body should be written,
5050
// or a closure telling if/where the response should be buffered based on its headers
51-
'on_progress' => null, // callable(int $dlNow, int $dlSize, array $info, ?Closure $resolve = null) - throwing any
52-
// exceptions MUST abort the request; it MUST be called on connection, on headers and on
53-
// completion; it SHOULD be called on upload/download of data and at least 1/s;
54-
// if passed, $resolve($host) / $resolve($host, $ip) can be called to read / populate
55-
// the DNS cache respectively
51+
'on_progress' => null, // callable(int $dlNow, int $dlSize, array $info) - throwing any exceptions MUST abort the
52+
// request; it MUST be called on connection, on headers and on completion; it SHOULD be
53+
// called on upload/download of data and at least 1/s
5654
'resolve' => [], // string[] - a map of host to IP address that SHOULD replace DNS resolution
5755
'proxy' => null, // string - by default, the proxy-related env vars handled by curl SHOULD be honored
5856
'no_proxy' => null, // string - a comma separated list of hosts that do not require a proxy to be reached

0 commit comments

Comments
 (0)