From 2a03d4fc4bc145d08e2eff6aa9acb0d40f94f0ac Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Sun, 31 May 2026 17:17:24 -0400 Subject: [PATCH 1/2] wip --- src/Imaging/GuzzleAdapter.php | 72 ++++++++---- src/Imaging/RemoteUrlValidator.php | 54 +++++++-- tests/Imaging/GuzzleAdapterTest.php | 117 ++++++++++++++++---- tests/Imaging/RemoteUrlValidatorTest.php | 134 +++++++++++++++++++++++ 4 files changed, 324 insertions(+), 53 deletions(-) create mode 100644 tests/Imaging/RemoteUrlValidatorTest.php diff --git a/src/Imaging/GuzzleAdapter.php b/src/Imaging/GuzzleAdapter.php index 13064f9ec5..fb0562bd41 100644 --- a/src/Imaging/GuzzleAdapter.php +++ b/src/Imaging/GuzzleAdapter.php @@ -6,6 +6,8 @@ use GuzzleHttp\ClientInterface; use GuzzleHttp\Exception\BadResponseException; use GuzzleHttp\Exception\ClientException; +use GuzzleHttp\Psr7\Uri; +use GuzzleHttp\Psr7\UriResolver; use League\Flysystem\Config; use League\Flysystem\FileAttributes; use League\Flysystem\FilesystemAdapter; @@ -14,6 +16,13 @@ class GuzzleAdapter implements FilesystemAdapter { + /** + * The maximum number of redirects to follow. + * + * @var int + */ + const MAX_REDIRECTS = 5; + /** * Whether this endpoint supports head requests. * @@ -149,18 +158,12 @@ public function visibility(string $path): FileAttributes protected function get($path) { try { - $response = $this->client->get($this->base.$path, $this->requestOptions()); - } catch (InvalidRemoteUrlException $e) { - throw $e; + $response = $this->send('GET', $this->base.$path); } catch (BadResponseException $e) { return false; } - if ($response->getStatusCode() !== 200) { - return false; - } - - return $response; + return $response->getStatusCode() === 200 ? $response : false; } /** @@ -176,7 +179,7 @@ protected function head($path) } try { - $response = $this->client->head($this->base.$path, $this->requestOptions()); + $response = $this->send('HEAD', $this->base.$path); } catch (ClientException $e) { if ($e->getResponse()->getStatusCode() === 405) { $this->supportsHead = false; @@ -185,27 +188,54 @@ protected function head($path) } return false; - } catch (InvalidRemoteUrlException $e) { - throw $e; } catch (BadResponseException $e) { return false; } - if ($response->getStatusCode() !== 200) { - return false; + return $response->getStatusCode() === 200 ? $response : false; + } + + /** + * Send a request, pinning the connection to the validated IP so the host + * cannot be rebound to an internal address between validation and the + * actual fetch. Redirects are followed manually so each hop is validated + * and pinned too. + */ + protected function send(string $method, string $url, int $redirects = 0) + { + // The connection is pinned to the validated IP via curl's CURLOPT_RESOLVE, + // which the stream handler has no equivalent for. Rather than silently fall + // back to an unpinned (rebindable) request, refuse to fetch without curl. + if (! $this->supportsConnectionPinning()) { + throw new \RuntimeException('The curl PHP extension is required to fetch remote images.'); + } + + $resolved = app(RemoteUrlValidator::class)->resolve($url); + + $response = $this->client->request($method, $url, [ + 'allow_redirects' => false, + 'curl' => [ + CURLOPT_RESOLVE => [sprintf('%s:%d:%s', $resolved['host'], $resolved['port'], implode(',', $resolved['ips']))], + ], + ]); + + $status = $response->getStatusCode(); + + if ($status >= 300 && $status < 400 && $response->hasHeader('Location')) { + if ($redirects >= self::MAX_REDIRECTS) { + throw new InvalidRemoteUrlException('Too many redirects.'); + } + + $location = UriResolver::resolve(new Uri($url), new Uri($response->getHeaderLine('Location'))); + + return $this->send($method, (string) $location, $redirects + 1); } return $response; } - protected function requestOptions() + protected function supportsConnectionPinning(): bool { - return [ - 'allow_redirects' => [ - 'on_redirect' => function ($request, $response, $uri) { - app(RemoteUrlValidator::class)->validate((string) $uri); - }, - ], - ]; + return extension_loaded('curl'); } } diff --git a/src/Imaging/RemoteUrlValidator.php b/src/Imaging/RemoteUrlValidator.php index 9ea428b1ff..73e8681a16 100644 --- a/src/Imaging/RemoteUrlValidator.php +++ b/src/Imaging/RemoteUrlValidator.php @@ -15,6 +15,39 @@ public function __construct(?callable $resolver = null) } public function parse($url) + { + $components = $this->validatedComponents($url); + + return [ + 'path' => Str::after($components['path'], '/'), + 'base' => $components['scheme'].'://'.$components['host'].$components['port_suffix'], + 'query' => $components['query'], + ]; + } + + public function validate($url) + { + $this->parse($url); + } + + /** + * Resolve and validate the URL host, returning the host, port, and the + * validated public IPs it resolves to. These IPs are intended to be pinned + * to the actual connection so the host cannot be rebound to an internal + * address between this check and the request being made. + */ + public function resolve($url) + { + $components = $this->validatedComponents($url); + + return [ + 'host' => $components['host'], + 'port' => $components['port'], + 'ips' => $components['ips'], + ]; + } + + protected function validatedComponents($url) { $parsed = parse_url($url); @@ -48,22 +81,19 @@ public function parse($url) throw new InvalidRemoteUrlException('Invalid URL host.'); } - $this->ensureHostResolvesToPublicIps($host); - - $port = isset($parsed['port']) ? ':'.$parsed['port'] : ''; + $ips = $this->ensureHostResolvesToPublicIps($host); return [ - 'path' => Str::after($parsed['path'] ?? '/', '/'), - 'base' => $scheme.'://'.$host.$port, + 'scheme' => $scheme, + 'host' => $host, + 'port' => $parsed['port'] ?? ($scheme === 'https' ? 443 : 80), + 'port_suffix' => isset($parsed['port']) ? ':'.$parsed['port'] : '', + 'path' => $parsed['path'] ?? '/', 'query' => $parsed['query'] ?? null, + 'ips' => $ips, ]; } - public function validate($url) - { - $this->parse($url); - } - protected function isValidHost($host) { return filter_var($host, FILTER_VALIDATE_IP) @@ -75,7 +105,7 @@ protected function ensureHostResolvesToPublicIps($host) if (filter_var($host, FILTER_VALIDATE_IP)) { $this->assertPublicIp($host); - return; + return [$host]; } $records = call_user_func($this->resolver, $host); @@ -90,6 +120,8 @@ protected function ensureHostResolvesToPublicIps($host) foreach ($ips as $ip) { $this->assertPublicIp($ip); } + + return $ips; } protected function assertPublicIp($ip) diff --git a/tests/Imaging/GuzzleAdapterTest.php b/tests/Imaging/GuzzleAdapterTest.php index 47710726ea..8e1a0bcd12 100644 --- a/tests/Imaging/GuzzleAdapterTest.php +++ b/tests/Imaging/GuzzleAdapterTest.php @@ -3,9 +3,7 @@ namespace Tests\Imaging; use GuzzleHttp\ClientInterface; -use GuzzleHttp\Psr7\Request; use GuzzleHttp\Psr7\Response; -use GuzzleHttp\Psr7\Uri; use Mockery; use PHPUnit\Framework\Attributes\Test; use Statamic\Exceptions\InvalidRemoteUrlException; @@ -23,6 +21,7 @@ public function setUp(): void return new RemoteUrlValidator(function ($host) { return match ($host) { 'example.com' => [['ip' => '93.184.216.34']], + 'cdn.example.com' => [['ip' => '93.184.216.35']], default => [], }; }); @@ -30,19 +29,42 @@ public function setUp(): void } #[Test] - public function it_allows_redirects_when_every_hop_is_public() + public function it_pins_the_connection_to_the_validated_ip() { $client = Mockery::mock(ClientInterface::class); - $client->shouldReceive('get')->once()->andReturnUsing(function ($url, $options) { - $this->assertEquals('https://example.com/foo.jpg', $url); - $this->assertArrayHasKey('allow_redirects', $options); - $this->assertArrayHasKey('on_redirect', $options['allow_redirects']); + $client->shouldReceive('request')->once()->andReturnUsing(function ($method, $url, $options) { + $this->assertSame('GET', $method); + $this->assertSame('https://example.com/foo.jpg', $url); + $this->assertFalse($options['allow_redirects']); + $this->assertSame(['example.com:443:93.184.216.34'], $options['curl'][CURLOPT_RESOLVE]); - $options['allow_redirects']['on_redirect']( - new Request('GET', 'https://example.com/foo.jpg'), - new Response(302, ['Location' => 'https://example.com/redirected/foo.jpg']), - new Uri('https://example.com/redirected/foo.jpg') - ); + return new Response(200, [], 'image-bytes'); + }); + + $adapter = new GuzzleAdapter('https://example.com', $client); + + $this->assertSame('image-bytes', $adapter->read('foo.jpg')); + } + + #[Test] + public function it_pins_the_connection_even_when_dns_would_rebind_after_validation() + { + // Simulates the DNS rebinding attack: validation sees a public IP, but a + // second lookup at connection time would resolve to localhost. Because we + // pin the validated IP via CURLOPT_RESOLVE, the connection can never use + // the rebound address — curl is told exactly which IP to connect to. + $rebindingResolver = function () { + static $calls = 0; + + return [['ip' => ++$calls === 1 ? '93.184.216.34' : '127.0.0.1']]; + }; + + $this->app->bind(RemoteUrlValidator::class, fn () => new RemoteUrlValidator($rebindingResolver)); + + $client = Mockery::mock(ClientInterface::class); + $client->shouldReceive('request')->once()->andReturnUsing(function ($method, $url, $options) { + // The pinned IP is the public one resolved during validation, not the rebound one. + $this->assertSame(['example.com:443:93.184.216.34'], $options['curl'][CURLOPT_RESOLVE]); return new Response(200, [], 'image-bytes'); }); @@ -52,19 +74,35 @@ public function it_allows_redirects_when_every_hop_is_public() $this->assertSame('image-bytes', $adapter->read('foo.jpg')); } + #[Test] + public function it_follows_redirects_and_pins_every_hop() + { + $client = Mockery::mock(ClientInterface::class); + + $client->shouldReceive('request')->once() + ->with('GET', 'https://example.com/foo.jpg', Mockery::on(function ($options) { + return $options['curl'][CURLOPT_RESOLVE] === ['example.com:443:93.184.216.34']; + })) + ->andReturn(new Response(302, ['Location' => 'https://cdn.example.com/foo.jpg'])); + + $client->shouldReceive('request')->once() + ->with('GET', 'https://cdn.example.com/foo.jpg', Mockery::on(function ($options) { + return $options['curl'][CURLOPT_RESOLVE] === ['cdn.example.com:443:93.184.216.35']; + })) + ->andReturn(new Response(200, [], 'image-bytes')); + + $adapter = new GuzzleAdapter('https://example.com', $client); + + $this->assertSame('image-bytes', $adapter->read('foo.jpg')); + } + #[Test] public function it_blocks_redirects_to_non_public_destinations() { $client = Mockery::mock(ClientInterface::class); - $client->shouldReceive('get')->once()->andReturnUsing(function ($url, $options) { - $options['allow_redirects']['on_redirect']( - new Request('GET', 'https://example.com/foo.jpg'), - new Response(302, ['Location' => 'http://169.254.169.254/latest/meta-data/']), - new Uri('http://169.254.169.254/latest/meta-data/') - ); - - return new Response(200, [], 'should-not-return'); - }); + $client->shouldReceive('request')->once()->andReturn( + new Response(302, ['Location' => 'http://169.254.169.254/latest/meta-data/']) + ); $adapter = new GuzzleAdapter('https://example.com', $client); @@ -73,4 +111,41 @@ public function it_blocks_redirects_to_non_public_destinations() $adapter->read('foo.jpg'); } + + #[Test] + public function it_refuses_to_fetch_when_curl_is_unavailable() + { + // Without curl the connection can't be pinned to the validated IP, so rather + // than silently fall back to a rebindable request we refuse to fetch at all. + $client = Mockery::mock(ClientInterface::class); + $client->shouldNotReceive('request'); + + $adapter = new class('https://example.com', $client) extends GuzzleAdapter + { + protected function supportsConnectionPinning(): bool + { + return false; + } + }; + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('curl PHP extension is required'); + + $adapter->read('foo.jpg'); + } + + #[Test] + public function it_stops_following_redirects_after_the_limit() + { + $client = Mockery::mock(ClientInterface::class); + $client->shouldReceive('request') + ->andReturn(new Response(302, ['Location' => 'https://example.com/foo.jpg'])); + + $adapter = new GuzzleAdapter('https://example.com', $client); + + $this->expectException(InvalidRemoteUrlException::class); + $this->expectExceptionMessage('Too many redirects.'); + + $adapter->read('foo.jpg'); + } } diff --git a/tests/Imaging/RemoteUrlValidatorTest.php b/tests/Imaging/RemoteUrlValidatorTest.php new file mode 100644 index 0000000000..fb54611366 --- /dev/null +++ b/tests/Imaging/RemoteUrlValidatorTest.php @@ -0,0 +1,134 @@ + match ($host) { + 'example.com' => [['ip' => '93.184.216.34']], + default => [], + }); + } + + #[Test] + public function it_resolves_the_host_port_and_validated_ips() + { + $this->assertSame([ + 'host' => 'example.com', + 'port' => 443, + 'ips' => ['93.184.216.34'], + ], $this->validator()->resolve('https://example.com/foo.jpg')); + } + + #[Test] + public function it_defaults_the_port_based_on_the_scheme() + { + $this->assertSame(443, $this->validator()->resolve('https://example.com/foo.jpg')['port']); + $this->assertSame(80, $this->validator()->resolve('http://example.com/foo.jpg')['port']); + } + + #[Test] + public function it_preserves_an_explicit_port() + { + $this->assertSame(8080, $this->validator()->resolve('http://example.com:8080/foo.jpg')['port']); + } + + #[Test] + public function it_returns_every_resolved_ip_so_they_can_all_be_pinned() + { + $resolved = $this->validator(fn () => [ + ['ip' => '93.184.216.34'], + ['ip' => '93.184.216.35', 'ipv6' => '2606:4700:4700::1111'], + ])->resolve('https://example.com/foo.jpg'); + + $this->assertSame(['93.184.216.34', '93.184.216.35', '2606:4700:4700::1111'], $resolved['ips']); + } + + #[Test] + public function it_resolves_an_ip_literal_host_to_itself_without_using_the_resolver() + { + $resolved = $this->validator(function () { + throw new \Exception('The resolver should not be called for an IP literal.'); + })->resolve('https://93.184.216.34/foo.jpg'); + + $this->assertSame([ + 'host' => '93.184.216.34', + 'port' => 443, + 'ips' => ['93.184.216.34'], + ], $resolved); + } + + #[Test] + public function it_lowercases_the_host() + { + $this->assertSame('example.com', $this->validator()->resolve('https://Example.COM/foo.jpg')['host']); + } + + #[Test] + public function it_blocks_hosts_that_resolve_to_non_public_ips() + { + $this->expectException(InvalidRemoteUrlException::class); + $this->expectExceptionMessage('Destination IP is not publicly routable.'); + + $this->validator(fn () => [['ip' => '127.0.0.1']])->resolve('https://internal.test/foo.jpg'); + } + + #[Test] + public function it_blocks_ip_literals_in_non_public_ranges() + { + $this->expectException(InvalidRemoteUrlException::class); + $this->expectExceptionMessage('Destination IP is not publicly routable.'); + + $this->validator()->resolve('http://169.254.169.254/latest/meta-data/'); + } + + #[Test] + public function it_blocks_hosts_that_cannot_be_resolved() + { + $this->expectException(InvalidRemoteUrlException::class); + $this->expectExceptionMessage('Unable to resolve URL host.'); + + $this->validator(fn () => [])->resolve('https://unknown.test/foo.jpg'); + } + + #[Test] + public function it_blocks_urls_with_credentials() + { + $this->expectException(InvalidRemoteUrlException::class); + $this->expectExceptionMessage('URLs with credentials are not allowed.'); + + $this->validator()->resolve('https://user:pass@example.com/foo.jpg'); + } + + #[Test] + public function it_blocks_non_http_schemes() + { + $this->expectException(InvalidRemoteUrlException::class); + $this->expectExceptionMessage('Only http and https URLs are allowed.'); + + $this->validator()->resolve('file:///etc/passwd'); + } + + #[Test] + public function it_parses_the_base_path_and_query() + { + $this->assertSame([ + 'path' => 'foo/bar.jpg', + 'base' => 'https://example.com', + 'query' => 'w=100', + ], $this->validator()->parse('https://example.com/foo/bar.jpg?w=100')); + } + + #[Test] + public function it_includes_an_explicit_port_in_the_parsed_base() + { + $this->assertSame('http://example.com:8080', $this->validator()->parse('http://example.com:8080/foo.jpg')['base']); + } +} From 5b89f50c9de9db5d2e636906f077a5c54b55b54f Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Tue, 2 Jun 2026 12:52:39 -0400 Subject: [PATCH 2/2] improve test --- tests/Imaging/GuzzleAdapterTest.php | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/Imaging/GuzzleAdapterTest.php b/tests/Imaging/GuzzleAdapterTest.php index 8e1a0bcd12..5fb9d2604f 100644 --- a/tests/Imaging/GuzzleAdapterTest.php +++ b/tests/Imaging/GuzzleAdapterTest.php @@ -47,23 +47,22 @@ public function it_pins_the_connection_to_the_validated_ip() } #[Test] - public function it_pins_the_connection_even_when_dns_would_rebind_after_validation() + public function it_resolves_the_host_once_and_pins_that_ip() { - // Simulates the DNS rebinding attack: validation sees a public IP, but a - // second lookup at connection time would resolve to localhost. Because we - // pin the validated IP via CURLOPT_RESOLVE, the connection can never use - // the rebound address — curl is told exactly which IP to connect to. - $rebindingResolver = function () { - static $calls = 0; - - return [['ip' => ++$calls === 1 ? '93.184.216.34' : '127.0.0.1']]; + // DNS rebinding works by returning a public IP for the validation lookup, + // then a private one for the connection's lookup. The fix resolves the host + // a single time and pins that IP to the connection via CURLOPT_RESOLVE, so + // the rebound answer below (127.0.0.1) is never reached. We assert both: the + // pin is the public IP, and the resolver is consulted exactly once. + $lookups = 0; + $resolver = function () use (&$lookups) { + return [['ip' => ++$lookups === 1 ? '93.184.216.34' : '127.0.0.1']]; }; - $this->app->bind(RemoteUrlValidator::class, fn () => new RemoteUrlValidator($rebindingResolver)); + $this->app->bind(RemoteUrlValidator::class, fn () => new RemoteUrlValidator($resolver)); $client = Mockery::mock(ClientInterface::class); $client->shouldReceive('request')->once()->andReturnUsing(function ($method, $url, $options) { - // The pinned IP is the public one resolved during validation, not the rebound one. $this->assertSame(['example.com:443:93.184.216.34'], $options['curl'][CURLOPT_RESOLVE]); return new Response(200, [], 'image-bytes'); @@ -72,6 +71,7 @@ public function it_pins_the_connection_even_when_dns_would_rebind_after_validati $adapter = new GuzzleAdapter('https://example.com', $client); $this->assertSame('image-bytes', $adapter->read('foo.jpg')); + $this->assertSame(1, $lookups, 'The host should be resolved once; the connection must reuse that result, not re-resolve.'); } #[Test]