[3.0][Testing] Cover the SNI regression in the unit suite - #9537
Open
albertlast wants to merge 1 commit into
Open
[3.0][Testing] Cover the SNI regression in the unit suite#9537albertlast wants to merge 1 commit into
albertlast wants to merge 1 commit into
Conversation
albertlast
force-pushed
the
tests/sni_fetch_safe
branch
from
August 19, 2026 19:32
2433e41 to
a2680ef
Compare
albertlast
force-pushed
the
tests/sni_fetch_safe
branch
from
August 20, 2026 03:57
a2680ef to
e98cbe4
Compare
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>
albertlast
force-pushed
the
tests/sni_fetch_safe
branch
from
August 28, 2026 22:12
e98cbe4 to
441a8c4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
#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:
and global, in both families.
localhost,.local,.internal,.test,.invalid,.exampleand.onioncan beasserted directly.
withResolvedHosts()helper seeds the cache
getIPs()keeps, in the same shape as the existingwithProxySettings(). 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
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.
getIPs()andresolvesTo()survive that round trip.
choose which one is used.
dns_get_record()never seesnames that only exist in the system's hosts file.
resolvesTo()matches on the address rather than its spelling —SMF\IPnormalises v6through
inet_ntop(inet_pton()), so expanded and compressed forms are one address.proxied()reaches its answer without asking the resolver, which matters because it runsfor every image in every post.
result()has something to return before it reads.What I could not reach, and deliberately left alone
CurlFetcher,SocketFetcherandFtpFetcher;CURLOPT_RESOLVE; the post-connectionaddress checks;
ProxyServer::checkRequest(). All need a socket. TheSubs-Compatshimsare out too — loading that file wants more of
Configthan the bootstrap sets, and itdefines 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-fixerclean.These still guard what they were written for. Putting
Sources/Url.phpback to the revisionof #9535 before the
$ascii_hostlocal went in:And
WebFetchResultTestfails the same way against aCurlFetcher::result()without thecount() - 1guard, with twoUndefined array key -1warnings.Issues References (Fixes|Related|Closes)