[HttpClient] Remove credentials from requests redirected to same host but different port

Author: GromNaN

CHANGELOG.md

@@ -5,6 +5,7 @@ CHANGELOG
 ---
 
  * Allow yielding `Exception` from MockResponse's `$body` to mock transport errors
+ * Remove credentials from requests redirected to same host but different port
 
 5.4
 ---

CurlHttpClient.php

@@ -90,6 +90,7 @@ public function request(string $method, string $url, array $options = []): Respo
         $scheme = $url['scheme'];
         $authority = $url['authority'];
         $host = parse_url($authority, \PHP_URL_HOST);
+        $port = parse_url($authority, \PHP_URL_PORT) ?: ('http:' === $scheme ? 80 : 443);
         $url = implode('', $url);
 
         if (!isset($options['normalized_headers']['user-agent'])) {
@@ -163,7 +164,6 @@ public function request(string $method, string $url, array $options = []): Respo
             // First reset any old DNS cache entries then add the new ones
             $resolve = $this->multi->dnsCache->evictions;
             $this->multi->dnsCache->evictions = [];
-            $port = parse_url($authority, \PHP_URL_PORT) ?: ('http:' === $scheme ? 80 : 443);
 
             if ($resolve && 0x072A00 > CurlClientState::$curlVersion['version_number']) {
                 // DNS cache removals require curl 7.42 or higher
@@ -293,7 +293,7 @@ public function request(string $method, string $url, array $options = []): Respo
             }
         }
 
-        return $pushedResponse ?? new CurlResponse($this->multi, $ch, $options, $this->logger, $method, self::createRedirectResolver($options, $host), CurlClientState::$curlVersion['version_number']);
+        return $pushedResponse ?? new CurlResponse($this->multi, $ch, $options, $this->logger, $method, self::createRedirectResolver($options, $host, $port), CurlClientState::$curlVersion['version_number']);
     }
 
     /**
@@ -373,11 +373,12 @@ private static function readRequestBody(int $length, \Closure $body, string &$bu
      *
      * Work around CVE-2018-1000007: Authorization and Cookie headers should not follow redirects - fixed in Curl 7.64
      */
-    private static function createRedirectResolver(array $options, string $host): \Closure
+    private static function createRedirectResolver(array $options, string $host, int $port): \Closure
     {
         $redirectHeaders = [];
         if (0 < $options['max_redirects']) {
             $redirectHeaders['host'] = $host;
+            $redirectHeaders['port'] = $port;
             $redirectHeaders['with_auth'] = $redirectHeaders['no_auth'] = array_filter($options['headers'], static function ($h) {
                 return 0 !== stripos($h, 'Host:');
             });
@@ -397,7 +398,8 @@ private static function createRedirectResolver(array $options, string $host): \C
             }
 
             if ($redirectHeaders && $host = parse_url('http:'.$location['authority'], \PHP_URL_HOST)) {
-                $requestHeaders = $redirectHeaders['host'] === $host ? $redirectHeaders['with_auth'] : $redirectHeaders['no_auth'];
+                $port = parse_url('http:'.$location['authority'], \PHP_URL_PORT) ?: ('http:' === $location['scheme'] ? 80 : 443);
+                $requestHeaders = $redirectHeaders['host'] === $host && $redirectHeaders['port'] === $port ? $redirectHeaders['with_auth'] : $redirectHeaders['no_auth'];
                 curl_setopt($ch, \CURLOPT_HTTPHEADER, $requestHeaders);
             }
 

NativeHttpClient.php

@@ -237,7 +237,7 @@ public function request(string $method, string $url, array $options = []): Respo
                 $url['authority'] = substr_replace($url['authority'], $ip, -\strlen($host) - \strlen($port), \strlen($host));
             }
 
-            return [self::createRedirectResolver($options, $host, $proxy, $info, $onProgress), implode('', $url)];
+            return [self::createRedirectResolver($options, $host, $port, $proxy, $info, $onProgress), implode('', $url)];
         };
 
         return new NativeResponse($this->multi, $context, implode('', $url), $options, $info, $resolver, $onProgress, $this->logger);
@@ -331,11 +331,11 @@ private static function dnsResolve(string $host, NativeClientState $multi, array
     /**
      * Handles redirects - the native logic is too buggy to be used.
      */
-    private static function createRedirectResolver(array $options, string $host, ?array $proxy, array &$info, ?\Closure $onProgress): \Closure
+    private static function createRedirectResolver(array $options, string $host, string $port, ?array $proxy, array &$info, ?\Closure $onProgress): \Closure
     {
         $redirectHeaders = [];
         if (0 < $maxRedirects = $options['max_redirects']) {
-            $redirectHeaders = ['host' => $host];
+            $redirectHeaders = ['host' => $host, 'port' => $port];
             $redirectHeaders['with_auth'] = $redirectHeaders['no_auth'] = array_filter($options['headers'], static function ($h) {
                 return 0 !== stripos($h, 'Host:');
             });
@@ -392,7 +392,7 @@ private static function createRedirectResolver(array $options, string $host, ?ar
 
             if (false !== (parse_url($location, \PHP_URL_HOST) ?? false)) {
                 // Authorization and Cookie headers MUST NOT follow except for the initial host name
-                $requestHeaders = $redirectHeaders['host'] === $host ? $redirectHeaders['with_auth'] : $redirectHeaders['no_auth'];
+                $requestHeaders = $redirectHeaders['host'] === $host && $redirectHeaders['port'] === $port ? $redirectHeaders['with_auth'] : $redirectHeaders['no_auth'];
                 $requestHeaders[] = 'Host: '.$host.$port;
                 $dnsResolve = !self::configureHeadersAndProxy($context, $host, $requestHeaders, $proxy, 'https:' === $url['scheme']);
             } else {

Tests/HttpClientTestCase.php

@@ -21,6 +21,8 @@
 use Symfony\Contracts\HttpClient\Exception\RedirectionExceptionInterface;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 use Symfony\Contracts\HttpClient\Test\HttpClientTestCase as BaseHttpClientTestCase;
+use Symfony\Contracts\HttpClient\Test\TestHttpServer;
+
 
 /*
 Tests for HTTP2 Push need a recent version of both PHP and curl. This docker command should run them:
@@ -406,4 +408,40 @@ public function testNullBody()
 
         $this->expectNotToPerformAssertions();
     }
+
+    /**
+     * @dataProvider getRedirectWithAuthTests
+     */
+    public function testRedirectWithAuth(string $url, bool $redirectWithAuth)
+    {
+        $p = TestHttpServer::start(8067);
+
+        try {
+            $client = $this->getHttpClient(__FUNCTION__);
+
+            $response = $client->request('GET', $url, [
+                'headers' => [
+                    'cookie' => 'foo=bar',
+                ],
+            ]);
+            $body = $response->toArray();
+        } finally {
+            $p->stop();
+        }
+
+        if ($redirectWithAuth) {
+            $this->assertArrayHasKey('HTTP_COOKIE', $body);
+        } else {
+            $this->assertArrayNotHasKey('HTTP_COOKIE', $body);
+        }
+    }
+
+    public function getRedirectWithAuthTests()
+    {
+        return [
+            'same host and port' => ['url' => 'http://localhost:8057/302', 'redirectWithAuth' => true],
+            'other port' => ['url' => 'http://localhost:8067/302', 'redirectWithAuth' => false],
+            'other host' => ['url' => 'http://127.0.0.1:8057/302', 'redirectWithAuth' => false],
+        ];
+    }
 }