Skip to content

[3.0][Testing] Cover the SNI regression in the unit suite - #9537

Open
albertlast wants to merge 1 commit into
SimpleMachines:release-3.0from
albertlast:tests/sni_fetch_safe
Open

[3.0][Testing] Cover the SNI regression in the unit suite#9537
albertlast wants to merge 1 commit into
SimpleMachines:release-3.0from
albertlast:tests/sni_fetch_safe

Conversation

@albertlast

@albertlast albertlast commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Description

Rebuilt on merged release-3.0. #9535 has landed, and both of the fixes this PR was
carrying went in with it, so they are gone from here — this is now tests only. The one
commit that is mine adds +301 lines across tests/Unit/UrlTest.php and a new
tests/Unit/WebFetchResultTest.php, and touches nothing in Sources/.

It is still stacked on #9511, which brings tests/bootstrap.php, phpunit.xml.dist and
the workflow. The only Sources/ line in the diff is #9511's own three-line docblock on
Url.php.

#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 the fix has landed.

Getting a deterministic answer without a resolver

Three routes, none of which touch the network:

  1. Addresses written out in the URL never need a lookup — loopback, private, link-local
    and global, in both families.
  2. 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.
  3. For anything that genuinely depends on what a name resolves to, a withResolvedHosts()
    helper seeds the cache getIPs() keeps, in the same shape as the existing
    withProxySettings(). That is what makes "resolves to a mix of global and private" and
    "resolves to nothing" assertable, with the same answer wherever the suite runs.

The cases worth naming

  • The URL comes back exactly as it went in. This is the regression itself: 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.
  • An internationalised host is still UTF-8 afterwards, and getIPs() and resolvesTo()
    survive that round trip.
  • One private address among several global ones refuses the lot, because we do not get to
    choose which one is used.
  • A name that resolves to nothing is not treated as safe. dns_get_record() never sees
    names that only exist in the system's hosts file.
  • resolvesTo() matches on the address rather than its spelling — SMF\IP normalises v6
    through inet_ntop(inet_pton()), so expanded and compressed forms are one address.
  • proxied() reaches its answer without asking the resolver, which matters because it runs
    for every image in every post.
  • result() has something to return before it reads.

What I could not reach, and deliberately left alone

CurlFetcher, SocketFetcher and FtpFetcher; CURLOPT_RESOLVE; the post-connection
address checks; ProxyServer::checkRequest(). All need a socket. The Subs-Compat shims
are out too — loading that file wants more of Config than the bootstrap sets, and it
defines several hundred functions into the global namespace with no way back. Those belong
in the integration suite, if anywhere.

Testing

Full suite against merged release-3.0: 177 tests, 258 assertions, green.
php-cs-fixer clean.

These still guard what they were written for. Putting Sources/Url.php back to the revision
of #9535 before the $ascii_host local went in:

1) UrlTest::testGetIPsSurvivesTheRoundTripThroughAscii
   TypeError: SMF\Url::getIPs(): Return value must be of type array, null returned
2) UrlTest::testGetIPsReturnsAnArrayForAnInternationalisedHostThatResolvesToNothing
   TypeError: SMF\Url::getIPs(): Return value must be of type array, null returned
3) UrlTest::testResolvesToWorksOnAnInternationalisedHost
   TypeError: SMF\Url::getIPs(): Return value must be of type array, null returned

   /app/Sources/Url.php:571   Undefined array key "münchen.smf-unit-tests"

And WebFetchResultTest fails the same way against a CurlFetcher::result() without the
count() - 1 guard, with two Undefined array key -1 warnings.

Issues References (Fixes|Related|Closes)

  1. Related [2.1|3.0]: make_fetch_safe breaks SNI required fetchs #9533
  2. Related [3.0] Fixes broken SNI resolution #9535
  3. Related [3.0][Testing] Cover last week's fixes that the unit suite can reach #9511

SimpleMachines#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 SimpleMachines#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 <noreply@anthropic.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants