From 441a8c406e27e1ec28a1c37444b982d725f8498d Mon Sep 17 00:00:00 2001 From: albertlast Date: Thu, 20 Aug 2026 05:57:00 +0200 Subject: [PATCH] Covers the SNI regression in the unit suite #9533 was a regression that shipped, and the decision the fetchers all hang off is reachable with no database and no network, so it seems worth holding onto now that #9535 has landed. The suite cannot reach the fetchers themselves. What it can reach is Url::isFetchSafe(), and three ways of getting a deterministic answer out of it without a resolver: - Addresses written out in the URL never need a lookup. That covers loopback, private, link local and global, in both families. - Reserved TLDs are refused on the name alone, before the resolver would be asked, so localhost, .local, .internal, .test, .invalid, .example and .onion can be asserted directly. - For anything that does depend on what a name resolves to, withResolvedHosts() seeds the cache that getIPs() keeps, in the same shape as the existing withProxySettings(). That is what lets us say a name resolves to a mix of global and private addresses, or to nothing at all, and get the same answer wherever the suite runs. The cases worth naming: that the URL comes back exactly as it went in, which is the regression itself; that an internationalised host is still UTF-8 afterwards, and that getIPs() and resolvesTo() survive that round trip; that one private address among several global ones refuses the lot; that a name resolving to nothing is not treated as safe; that resolvesTo() matches on the address rather than its spelling; that proxied() reaches its answer without asking the resolver; and that result() has something to return before it reads. Out of reach, and left alone: the three fetchers, CURLOPT_RESOLVE, the post-connection address checks, and ProxyServer::checkRequest(), all of which need a socket. The Subs-Compat shims are out too, since loading that file wants more of Config than the bootstrap sets and defines several hundred functions into the global namespace with no way back. Co-Authored-By: Claude Opus 5 Signed-off-by: albertlast --- tests/Unit/UrlTest.php | 246 ++++++++++++++++++++++++++++++ tests/Unit/WebFetchResultTest.php | 55 +++++++ 2 files changed, 301 insertions(+) create mode 100644 tests/Unit/WebFetchResultTest.php diff --git a/tests/Unit/UrlTest.php b/tests/Unit/UrlTest.php index db7888de94..da226b0cd4 100644 --- a/tests/Unit/UrlTest.php +++ b/tests/Unit/UrlTest.php @@ -8,6 +8,7 @@ use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use SMF\Config; +use SMF\IP; use SMF\Url; #[CoversClass(Url::class)] @@ -136,6 +137,146 @@ public function testProxiedLeavesUnroutableHostsAlone(string $url, bool $expecte }); } + #[DataProvider('fetchSafeLiteralProvider')] + public function testIsFetchSafeJudgesLiteralAddresses(string $url, bool $expected): void + { + $this->assertSame($expected, Url::create($url)->isFetchSafe(['http', 'https'])); + } + + #[DataProvider('fetchSafeReservedTldProvider')] + public function testIsFetchSafeRejectsReservedTlds(string $url): void + { + // These never reach the resolver: they are refused on the name alone, + // which is the only reason this case can live in a unit test. + $this->assertFalse(Url::create($url)->isFetchSafe(['http', 'https'])); + } + + #[DataProvider('fetchSafeSchemeProvider')] + public function testIsFetchSafeHonoursTheAllowedSchemes(string $url, array $schemes, bool $expected): void + { + $this->assertSame($expected, Url::create($url)->isFetchSafe($schemes)); + } + + public function testIsFetchSafeWithNoSchemesAllowsAnythingWeCanFetch(): void + { + $this->assertTrue(Url::create('http://93.184.216.34/x')->isFetchSafe()); + $this->assertTrue(Url::create('ftp://93.184.216.34/x')->isFetchSafe()); + $this->assertFalse(Url::create('javascript:alert(1)')->isFetchSafe()); + } + + public function testIsFetchSafeLeavesTheUrlAlone(): void + { + // The whole point of the exercise. When the host was swapped for a + // literal address, the request went out with the wrong SNI name, the + // wrong Host header, and a certificate that could not match. + $url = new Url('https://93.184.216.34:8443/a/b?c=d#e'); + + $url->isFetchSafe(['http', 'https']); + + $this->assertSame('https://93.184.216.34:8443/a/b?c=d#e', (string) $url); + $this->assertSame('93.184.216.34', $url->host); + } + + public function testIsFetchSafeLeavesAnInternationalisedHostInUtf8(): void + { + $url = new Url('https://münchen.smf-unit-tests/straße'); + + $this->withResolvedHosts(['xn--mnchen-3ya.smf-unit-tests' => ['93.184.216.34']], function () use ($url): void { + $this->assertTrue($url->isFetchSafe(['http', 'https'])); + }); + + $this->assertSame('https://münchen.smf-unit-tests/straße', (string) $url); + } + + #[DataProvider('fetchSafeResolutionProvider')] + public function testIsFetchSafeJudgesWhatTheHostResolvesTo(array $ips, bool $expected): void + { + $this->withResolvedHosts(['resolved.smf-unit-tests' => $ips], function () use ($expected): void { + $this->assertSame($expected, Url::create('http://resolved.smf-unit-tests/x')->isFetchSafe(['http', 'https'])); + }); + } + + public function testResolvesToMatchesAnyOfTheKnownAddresses(): void + { + $this->withResolvedHosts(['resolved.smf-unit-tests' => ['93.184.216.34', '93.184.216.35']], function (): void { + $url = Url::create('http://resolved.smf-unit-tests/x'); + + $this->assertTrue($url->resolvesTo(new IP('93.184.216.34'))); + $this->assertTrue($url->resolvesTo(new IP('93.184.216.35'))); + $this->assertFalse($url->resolvesTo(new IP('93.184.216.36'))); + }); + } + + public function testResolvesToComparesAddressesRatherThanTheirWrittenForm(): void + { + // SMF\IP puts v6 addresses through inet_ntop(inet_pton()), so the + // expanded and the compressed spelling are the same address by the + // time we compare them. + $url = new Url('http://[2606:4700:4700::1111]/x'); + + $this->assertTrue($url->resolvesTo(new IP('2606:4700:4700:0000:0000:0000:0000:1111'))); + $this->assertFalse($url->resolvesTo(new IP('2606:4700:4700::1112'))); + } + + public function testGetIPsSurvivesTheRoundTripThroughAscii(): void + { + // getIPs() punycodes the host, looks that up, and then puts the URL + // back into UTF-8 before returning. Both conversions re-parse the URL, + // so the host is spelled one way going in and another coming out, and + // reaching for the answer by host name afterwards reaches for a key + // that was never written. + $url = new Url('https://münchen.smf-unit-tests/x'); + + $this->withResolvedHosts(['xn--mnchen-3ya.smf-unit-tests' => ['93.184.216.34']], function () use ($url): void { + $ips = $url->getIPs(); + + $this->assertCount(1, $ips); + $this->assertSame('93.184.216.34', (string) $ips[0]); + }); + + // And the URL is still spelled the way we wrote it. + $this->assertSame('https://münchen.smf-unit-tests/x', (string) $url); + $this->assertSame('münchen.smf-unit-tests', $url->host); + } + + public function testGetIPsReturnsAnArrayForAnInternationalisedHostThatResolvesToNothing(): void + { + $url = new Url('https://münchen.smf-unit-tests/x'); + + $this->withResolvedHosts(['xn--mnchen-3ya.smf-unit-tests' => []], function () use ($url): void { + $this->assertSame([], $url->getIPs()); + }); + } + + public function testResolvesToWorksOnAnInternationalisedHost(): void + { + // get_ips_for_url() and url_resolves_to() reach getIPs() and + // resolvesTo() without converting the URL first, so this is the shape + // the compatibility layer hands them. + $url = new Url('https://münchen.smf-unit-tests/x'); + + $this->withResolvedHosts(['xn--mnchen-3ya.smf-unit-tests' => ['93.184.216.34']], function () use ($url): void { + $this->assertTrue($url->resolvesTo(new IP('93.184.216.34'))); + $this->assertFalse($url->resolvesTo(new IP('10.0.0.1'))); + }); + } + + public function testProxiedDoesNotConsultTheResolver(): void + { + // proxied() runs for every image in every post, and only decides whether + // to route through proxy.php, which does its own checking. An empty + // answer here would read as "unsafe" to isFetchSafe(), so if this comes + // back proxied then proxied() never asked. + $this->withResolvedHosts(['images.smf-unit-tests' => []], function (): void { + $this->withProxySettings(function (): void { + $this->assertStringStartsWith( + 'https://forum.test-site.com/forum/proxy.php?request=', + (string) Url::create('http://images.smf-unit-tests/pic.png')->proxied(), + ); + }); + }); + } + /*********************** * Public static methods ***********************/ @@ -180,6 +321,79 @@ public static function proxiedProvider(): array ]; } + /** + * Addresses written out in the URL, so no lookup happens at all. + * + * @return array + */ + public static function fetchSafeLiteralProvider(): array + { + return [ + 'ipv4 loopback' => ['http://127.0.0.1/x', false], + 'ipv4 private' => ['http://10.0.0.1/x', false], + 'ipv4 private 192' => ['http://192.168.1.1/x', false], + 'ipv4 link local' => ['http://169.254.169.254/x', false], + 'ipv4 global' => ['http://93.184.216.34/x', true], + 'ipv6 loopback' => ['http://[::1]/x', false], + 'ipv6 unique local' => ['http://[fd00::1]/x', false], + 'ipv6 documentation' => ['http://[2001:db8::1]/x', false], + 'ipv6 global' => ['http://[2606:4700:4700::1111]/x', true], + ]; + } + + /** + * Names that are never in public DNS, per RFC 2606 and RFC 6761. + * + * @return array + */ + public static function fetchSafeReservedTldProvider(): array + { + return [ + 'localhost' => ['http://localhost/x'], + 'local' => ['http://box.local/x'], + 'internal' => ['http://box.internal/x'], + 'test' => ['http://box.test/x'], + 'invalid' => ['http://box.invalid/x'], + 'example' => ['http://box.example/x'], + 'onion' => ['http://box.onion/x'], + ]; + } + + /** + * @return array, bool}> + */ + public static function fetchSafeSchemeProvider(): array + { + return [ + 'http when http is allowed' => ['http://93.184.216.34/x', ['http', 'https'], true], + 'ftp when only http is allowed' => ['ftp://93.184.216.34/x', ['http', 'https'], false], + 'ftp when ftp is allowed' => ['ftp://93.184.216.34/x', ['ftp', 'ftps'], true], + 'no scheme' => ['//93.184.216.34/x', ['http', 'https'], false], + 'no host' => ['mailto:someone@example.com', ['http', 'https'], false], + ]; + } + + /** + * What a name resolves to decides the answer. Every address has to be + * globally routable, because we will not know which one gets used. + * + * @return array, bool}> + */ + public static function fetchSafeResolutionProvider(): array + { + return [ + 'all global' => [['93.184.216.34', '93.184.216.35'], true], + 'all private' => [['10.0.0.1'], false], + 'one private among global' => [['93.184.216.34', '127.0.0.1'], false], + 'global v6' => [['2606:4700:4700::1111'], true], + 'link local v6' => [['fe80::1'], false], + // Nothing came back. dns_get_record() never sees names that only + // exist in the system's hosts file, so this is not proof that the + // name is harmless. + 'nothing at all' => [[], false], + ]; + } + /****************** * Internal methods ******************/ @@ -195,6 +409,38 @@ public static function proxiedProvider(): array * * @param callable $test The assertions to run. */ + /** + * Runs $test with the given hosts already resolved, then puts the cache + * back as it was. + * + * Url::getIPs() remembers what a host resolved to for the rest of the + * request. Seeding that cache is what lets a unit test say what a name + * resolves to without a resolver, without a network, and without a result + * that changes depending on where the suite is run. + * + * @param array> $hosts Host name to addresses. + * @param callable $test The assertions to run. + */ + protected function withResolvedHosts(array $hosts, callable $test): void + { + $property = new \ReflectionProperty(Url::class, 'ips'); + + // Typed, and declared with no default, so it starts out uninitialised + // and reading it before anything has resolved would throw. + $previous = $property->isInitialized() ? $property->getValue() : []; + + $property->setValue(null, array_map( + fn(array $ips): array => array_map(fn(string $ip): IP => new IP($ip), $ips), + $hosts, + )); + + try { + $test(); + } finally { + $property->setValue(null, $previous); + } + } + protected function withProxySettings(callable $test): void { $enabled = Config::$image_proxy_enabled ?? false; diff --git a/tests/Unit/WebFetchResultTest.php b/tests/Unit/WebFetchResultTest.php new file mode 100644 index 0000000000..d9670488e0 --- /dev/null +++ b/tests/Unit/WebFetchResultTest.php @@ -0,0 +1,55 @@ +assertNull($fetcher->result('success')); + $this->assertNull($fetcher->result('body')); + $this->assertNull($fetcher->result()); + } + + /*********************** + * Public static methods + ***********************/ + + /** + * @return array + */ + public static function fetcherProvider(): array + { + return [ + 'curl' => [CurlFetcher::class], + 'socket' => [SocketFetcher::class], + ]; + } +}