[3.0][Testing] Cover the SNI regression in the unit suite - #9537
Open
albertlast wants to merge 21 commits into
Open
[3.0][Testing] Cover the SNI regression in the unit suite#9537albertlast wants to merge 21 commits into
albertlast wants to merge 21 commits into
Conversation
SMF has no automated tests. CI proves that the code parses and that it is formatted; it never executes anything. Every one of the bugs fixed in SimpleMachines#9319 through SimpleMachines#9322 parsed cleanly and passed every check. Quite a lot of 3.0 is reachable without a forum behind it. The bootstrap here defines the constants index.php would define and points the autoloader at Sources/, and that is enough: no Settings.php, no database, no request. Anything that reaches Config::$modSettings, User::$me or Db::$db is out of scope and belongs in an integration suite. The first tests cover ground that recently broke: - ActionTrait::load() returning an instance of the class it was called on, in both orders and in two separate class hierarchies. - CreatePost_Notify::getTimeOffset(), including the half-hour and quarter-hour zones that an int cast used to truncate. - Utils::buildRegex(), including the trailing quoted character from SimpleMachines#9318. tests/ is already excluded from the license header check in BuildTools, and the directory index.php files keep check-smf-index happy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a PHPUnit workflow across the same 8.4 and 8.5 matrix the syntax check already uses, and a composer test script. Two adjustments fall out of running the suite next to the existing checks. The PHPUnit cache lives in .phpunit.cache rather than under cache/, because check-smf-index walks every directory that is not hidden and would otherwise report a missing index file the moment anyone runs the tests locally. And AGENTS.md no longer says there is no test suite; it now says what the suite does and does not cover, so an agent does not mistake a green run for proof that a change works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first pass covered only the three things that had recently broken. Quite a lot more is reachable without a database once the bootstrap sets the paths and default language that the Unicode and entity helpers use to find their data files, which is six lines and still reads nothing from Settings.php. Adds coverage for Utils' entity-aware string handling and Unicode case conversion, IP, Url, Uuid, Sapi, Security's password hashing, Punycode and TimeInterval. 99 tests, 144 assertions, on 8.4 and 8.5. Two behaviours are deliberately described rather than asserted, because pinning them down would preserve something that looks wrong: - Sapi::memoryReturnBytes() strips the last character before parsing, so a unit-less value such as '128' reads as 12 and the '-1' that means "no limit" reads as 0. Only suffixed values are asserted. - Url::isScheme() compares the scheme without normalising case, so an uppercase scheme fails to match its own name. Only exact-case matching is asserted. IP's constructor accepts the packed binary form, which it cannot tell apart from any other 4 or 16 byte string, so 'nope' becomes 110.111.112.101. That one is genuine ambiguity rather than a defect, so it is pinned down as a test in its own right. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RFC 3986, section 3.1, makes scheme names case insensitive, and this class keeps the scheme exactly as it was written rather than normalizing it. isScheme() compared the two with in_array(), so a URL written with an uppercase scheme did not match its own name. That reaches two callers. isWebsite() stops recognising HTTP:// and HTTPS:// as websites, and the avatar handling in User treats a DATA: URI as though it were a remote address. Folds both sides before comparing, and makes the comparison strict while it is there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The memoryReturnBytes() and isScheme() cases were described in comments rather than asserted, because pinning down the behaviour would have preserved it. Now that both are fixed, they become tests. Verified to fail against the unfixed code: reverting the two source files alone fails exactly these six tests and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CoversClass on a trait is not a valid coverage target, and PHPUnit only says so when coverage is actually collected. The suite passed on its own and failed all five ActionTrait cases the moment anyone ran it with --coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com> # Conflicts: # AGENTS.md
The testing notes described the suite mainly as a limitation, which left agents with no way to tell whether the code in front of them was reachable from it. Sets out the expectation that a reachable change carries a test, and lists the cases that work with the examples already in tests/Unit/: pure helpers, value objects, class-level behaviour, protected helpers through reflection, and modSettings keys the test sets itself. Also names the strict-mode traps and the two ways the style fixer rearranges a test file. Corrects the CI claim as well; phpunit.yml only runs on pull requests and on pushes to release-3.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Three of the bug fixes merged into release-3.0 over the past week touch code that needs no database, so they can be pinned down here rather than only in a running forum. SimpleMachines#9506 stopped SMF\IP::ip2range() validating the two ends of a range as IPv4 only, which meant an IPv6 range was not recognised as a range at all and came back as 255.255.255.255. SimpleMachines#9507 made SMF\Url::proxied() strip the brackets off an IPv6 host before asking filter_var() about it. Without that, every literal IPv6 host read as a name rather than an address, so the private and reserved ranges were not excluded and the proxy would fetch them. SimpleMachines#9499 stopped SMF\TimeInterval holding a \DateInterval of its own and started being one, which is what \DateTime::add() and ::sub() read. The two cases here that asserted the old canonical string form are updated: a duration naming no years or months no longer carries a zero day count through stringification. Also covers SMF\IP::matchToCIDR(), which SimpleMachines#9507 restructured without meaning to change it. Those cases pass either side of that commit, which is the point. Three notes on what is deliberately absent. TimeInterval::localize() goes through Lang::getTxt(), which loads a language file and so wants Theme and Db::$db. MarkdownParser, for SimpleMachines#9509, builds its parent, which calls User::setMe(). Neither is reachable from this bootstrap. The third is a defect rather than a boundary. matchToCIDR() builds its IPv6 mask with str_repeat('f', (int) $cidr_subnetmask / 4), where the cast binds to the subnet mask rather than to the division, so any prefix length that is not a multiple of four hands str_repeat() a float and throws a TypeError. The switch immediately below it, which exists to add the odd nibble for exactly those prefixes, has therefore never run. The data provider says so in a comment and stays on multiples of four: asserting the TypeError would only preserve it. Signed-off-by: Mathias Alberts <mathiaspapealbert@hotmail.com> Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
d79501a went back to calling parent::__construct(), so $days is false for any interval built from a string and __toString() no longer invents a zero day component to go with it. 'PT30M' now stays 'PT30M', which is what ISO 8601 asks for and what the old P0DT30M was only ever an artefact of. The round trip test asserted that artefact, so it now asserts the normalisation instead: a redundant P0D handed in is not written back out. Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com> # Conflicts: # tests/Unit/TimeIntervalTest.php
albertlast
force-pushed
the
tests/sni_fetch_safe
branch
from
August 19, 2026 19:32
2433e41 to
a2680ef
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 20, 2026 03:57
a2680ef to
e98cbe4
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
2 participants
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)