Enhance redirect handling: preserve HEAD method for 303 responses and reject malicious redirect schemes

Author: Grotax

src/FeedIo/Adapter/Http/Client.php

@@ -89,9 +89,10 @@ protected function request(
                     $duration
                 );
             case 303:
-                // 303 See Other always requires GET
+                // 303 See Other: change POST/PUT/DELETE to GET, but preserve HEAD
+                $redirectMethod = $method === 'HEAD' ? 'HEAD' : 'GET';
                 return $this->handleRedirect(
-                    'GET',
+                    $redirectMethod,
                     $url,
                     $psrResponse,
                     $modifiedSince,
@@ -113,7 +114,7 @@ protected function request(
      * @param \Psr\Http\Message\ResponseInterface $psrResponse
      * @param DateTime|null $modifiedSince
      * @param int $redirectCount
-     * @param float $duration
+     * @param float $duration Duration of the redirect request (used for error reporting)
      * @return ResponseInterface
      * @throws ClientExceptionInterface
      */
@@ -143,11 +144,19 @@ protected function handleRedirect(
      * @param string $currentUrl
      * @param string $location
      * @return string
+     * @throws ServerErrorException
      */
     protected function resolveRedirectUrl(string $currentUrl, string $location): string
     {
-        // If location is already absolute, return it
-        if (preg_match('/^https?:\/\//i', $location)) {
+        // Check if location has a scheme (absolute URL or potentially malicious scheme)
+        if (preg_match('/^([a-z][a-z0-9+.-]*):(?:\/\/)?/i', $location, $matches)) {
+            $scheme = strtolower($matches[1]);
+            if (!in_array($scheme, ['http', 'https'], true)) {
+                throw new ServerErrorException(
+                    new \Nyholm\Psr7\Response(400, [], 'Invalid redirect scheme: ' . $scheme),
+                    0
+                );
+            }
             return $location;
         }
 

tests/FeedIo/Adapter/Http/ClientTest.php

@@ -223,4 +223,86 @@ public function testResponseDurationIsTrackedOnError(): void
             $this->assertGreaterThanOrEqual(0, $e->getDuration());
         }
     }
+
+    public function test303RedirectPreservesHeadMethod(): void
+    {
+        // Directly test that a 303 redirect preserves HEAD method
+        // We'll use reflection to call the protected request() method with HEAD
+        
+        $redirectResponse = new PsrResponse(303, ['Location' => 'https://example.com/new-feed.xml']);
+        $finalResponse = new PsrResponse(200, [], 'content');
+
+        $requestCount = 0;
+        $this->psrClient
+            ->expects($this->exactly(2))
+            ->method('sendRequest')
+            ->willReturnCallback(function (RequestInterface $request) use ($redirectResponse, $finalResponse, &$requestCount) {
+                $requestCount++;
+                
+                if ($requestCount === 1) {
+                    // First request: HEAD
+                    $this->assertEquals('HEAD', $request->getMethod());
+                    return $redirectResponse;
+                }
+                
+                // Second request: should still be HEAD (not changed to GET for 303)
+                $this->assertEquals('HEAD', $request->getMethod());
+                $this->assertEquals('https://example.com/new-feed.xml', (string) $request->getUri());
+                return $finalResponse;
+            });
+
+        // Use reflection to call protected request() method with HEAD
+        $reflection = new \ReflectionClass($this->client);
+        $method = $reflection->getMethod('request');
+        $method->setAccessible(true);
+        
+        $response = $method->invoke($this->client, 'HEAD', 'https://example.com/old-feed.xml', null, 0);
+        
+        $this->assertEquals(200, $response->getStatusCode());
+    }
+
+    /**
+     * @dataProvider maliciousSchemeProvider
+     */
+    public function testRejectsRedirectsWithMaliciousSchemes(string $maliciousUrl): void
+    {
+        $redirectResponse = new PsrResponse(301, ['Location' => $maliciousUrl]);
+
+        $this->psrClient
+            ->expects($this->once())
+            ->method('sendRequest')
+            ->willReturn($redirectResponse);
+
+        $this->expectException(ServerErrorException::class);
+
+        $this->client->getResponse('https://example.com/feed.xml');
+    }
+
+    public static function maliciousSchemeProvider(): array
+    {
+        return [
+            'file scheme' => ['file:///etc/passwd'],
+            'ftp scheme' => ['ftp://malicious.com/data'],
+            'javascript scheme' => ['javascript:alert(1)'],
+            'data scheme' => ['data:text/html,<script>alert(1)</script>'],
+            'mailto scheme' => ['mailto:[email protected]'],
+            'tel scheme' => ['tel:+1234567890'],
+        ];
+    }
+
+    public function testAllowsHttpAndHttpsRedirects(): void
+    {
+        $httpRedirect = new PsrResponse(301, ['Location' => 'http://example.com/feed.xml']);
+        $httpsRedirect = new PsrResponse(301, ['Location' => 'https://secure.example.com/feed.xml']);
+        $finalResponse = new PsrResponse(200, [], 'content');
+
+        $this->psrClient
+            ->expects($this->exactly(3))
+            ->method('sendRequest')
+            ->willReturnOnConsecutiveCalls($httpRedirect, $httpsRedirect, $finalResponse);
+
+        $response = $this->client->getResponse('https://example.com/feed.xml');
+
+        $this->assertEquals(200, $response->getStatusCode());
+    }
 }