[3.0] Fixes broken SNI resolution

[Sesquipedalian]

Fixes #9533 for 3.0

[Sesquipedalian]

Seems to work, but please test to confirm, @jdarwood007.

[sbulen]

I just tried an install, get this at the end of the install:

Error logged by the install is:

Could not retrieve the file https ://www.simplemachines.org/smf/current-version.js?version=SMF+3.0+Alpha+4.

When running from Scheduled Tasks, you get this additional detail:

http: //localhost/84van30/index.php?action=admin;area=scheduledtasks
[D:/wamp64/www/84van30/Sources/WebFetch/WebFetchApi.php](http: //localhost/84van30/index.php?action=admin;area=logs;sa=errorlog;file=RDovd2FtcDY0L3d3dy84NHZhbjMwL1NvdXJjZXMvV2ViRmV0Y2gvV2ViRmV0Y2hBcGkucGhw;line=133) (Line 133)
1024: SMF\WebFetch\WebFetchApi::fetch: Bad URL

[albertlast]

@Sesquipedalian I went looking for why @sbulen was getting Bad URL, and there is a
straightforward reason plus a few more things behind it. I've put a fixed version up as
#9536 — it's this branch with three commits on top, so if this one merges first only those
three remain.

The approach here is right, to be clear. Swapping the host for a literal IP was always
going to take out SNI, name-based virtual hosting and certificate validation together, and
validating the name while pinning the connection is the correct replacement.

Why nothing fetches at all. Url::isFetchSafe() reads $url throughout, but the
variable is never assigned — it's $this:

public function isFetchSafe(array $allowed_schemes): bool
{
	$is_ascii = $this->is_ascii;
	$this->toAscii();

	if (
		empty($url->scheme)                       // <- $url is undefined
		|| !\in_array($url->scheme, $allowed_schemes)
		|| empty($url->host)
		|| preg_match('/.../', $url->host)

empty() on an undefined variable is true, so the method always returns false and the
guard rejects every URL before a connection is attempted. That is sbulen's error exactly.

Then five more, which the first one hides.

1. ProxyServer::checkRequest() has the test inverted. It was ($request = WebFetchApi::makeSafe($request)) === null, reject when unsafe; it's now || $request->isFetchSafe(['http','https']), reject when safe. proxy.php refuses every
   public image and fetches the private addresses the check exists to prevent.
2. Url::proxied() calls $proxied->isFetchSafe() with no argument, but the signature has
   no default — ArgumentCountError on every proxied image.
3. CurlFetcher::redirect() still calls WebFetchApi::makeSafe(), which this PR deletes.
   Any 301/302/307 is a fatal error.
4. CurlFetcher and FtpFetcher don't import SMF\IP, so new IP(...) resolves to
   SMF\WebFetch\APIs\IP and fatals as soon as the new check is reached. SocketFetcher
   has the import; those two don't.
5. is_fetch_safe() passes [], which matches nothing, so it always returns false. An
   empty list meant "any scheme with a handler" under makeSafe(); isFetchSafe() has no
   such fallback.

Two design points worth your view, which is really why I opened a PR rather than just
listing this.

CURLOPT_RESOLVE beats checking primary_ip afterwards. The post-connection compare
only tells us where we already went, and it's only sound because the $ips cache happens
to be warm from the pre-connect isFetchSafe() — an invisible dependency, and one that
isn't warm for redirect targets, which are fresh Url objects. CURLOPT_RESOLVE tells
curl up front which address it may use for the host, so the URL keeps its name and SNI, the
Host header and cert validation all still see the real one, with no window at all. I left
SocketFetcher's connect-then-verify alone — the exposure there is a handshake with no
request data sent, and stream_socket_client with an SSL peer_name context is easy to
get subtly wrong.

Url::proxied() shouldn't touch the resolver. isFetchSafe() does a
dns_get_record(), and proxied() runs for every image in every post — a topic with
twenty images becomes twenty blocking lookups on render. It also only decides whether to
route through proxy.php, which does its own checking, so a DNS hiccup silently stopping
proxying is a poor trade. I put the cheap literal-IP and reserved-TLD test back.

Smaller things I also changed

• Dropping WebFetchApi::$resolved_hosts costs more than lookups. That cache is what makes
  the post-connection checks mean anything — they have to compare against what was resolved
  before connecting, or a rebinding attack simply answers twice. I moved it into
  getIPs() as a static, and made parse() drop it since the host may change.
• $ips === array_filter($ips) is true for an empty $ips, and dns_get_record() never
  sees names that only exist in the hosts file — so http://someinternalbox/ passes the
  guard.
• resolvesTo() uses == on the objects, which also weighs IP::$host; either side may
  have filled that in with a reverse lookup. Comparing the addresses is safer.
• Removing the PASV check from FtpConnection::passive() only replaces it for FtpFetcher.
  The package manager and the maintenance tools build FtpConnection directly in twelve
  places and never go through FtpFetcher, and create_file()/list_dir() both
  fsockopen() straight to $this->pasv['ip'].
• make_fetch_safe() is a 2.1 function and Subs-Compat.php exists to keep 2.1 names
  callable, so removing it fatals any mod calling it. I brought it back deprecated,
  returning the URL unchanged when safe and null when not — and what comes back is now
  fetchable, which it wasn't before.
• Typo: "any of those I{ addresses" in the resolvesTo() docblock.

Verification. I ran the URL from the issue through both fetchers, on release-3.0
first and then on the fixed branch. Before:

===== CurlFetcher =====
  SNI host, real fetch      FAIL  code='0' len=0
===== SocketFetcher =====
  SNI host, real fetch      FAIL
    'url' => 'https://172.67.70.118/smf/current-version.js?version=SMF+3.0+Alpha+4'

After:

===== CurlFetcher =====
  SNI host, real fetch                SUCCESS  code='200' len=38
  http://127.0.0.1/ refused           OK
  follows http->https redirect        OK  code='403' len=5520
===== SocketFetcher =====
  SNI host, real fetch                SUCCESS  code=200 len=38
  http://127.0.0.1/ refused           OK

Plus 20 checks over isFetchSafe()/resolvesTo()/the compat shims and 6 over proxied(),
all passing, and php-cs-fixer clean on all seven files.

Unrelated, and I deliberately left it: SocketFetcher returns a 400 when it follows an
http→https redirect. It does that identically on unmodified release-3.0, so it's
pre-existing and wants its own issue.

Happy for you to cherry-pick whichever parts you want rather than take #9536 whole —
particularly the proxied() revert and the result() warning fix, which are judgement
calls rather than corrections.

[Sesquipedalian]

How about now, @sbulen?

[albertlast]

The new revision picks up nearly all of it, so I've closed #9536 off in my head — this is
the version to go with. I rebuilt #9537 (unit tests) on top of this branch instead.

Writing those tests turned up one thing that is live here right now, and it is a fatal.

getIPs() is fatal for any internationalised host

getIPs() punycodes the host, resolves that, and 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 the return statement reaches for a key that was never written:

public function getIPs(): array
{
	if (isset(self::$ips[$this->host])) {          // "münchen.de"
	…
	$this->toAscii();                              // host is now "xn--mnchen-3ya.de"
	self::$ips[$this->host] = [];                  // written under the ASCII name
	…
	if (!$is_ascii) {
		$this->toUtf8();                           // host is back to "münchen.de"
	}

	return self::$ips[$this->host];                // never written under this one
}

Reproduced with no network at all — the host does not resolve, and the fault is in which
key gets read back, not in what the resolver said:

getIPs       on a UTF-8 host -> TypeError: SMF\Url::getIPs(): Return value must be
                                of type array, null returned
resolvesTo   on a UTF-8 host -> TypeError: ...
isFetchSafe  on a UTF-8 host -> false

isFetchSafe() is fine, because it converts the URL before it asks — which is why the main
fetch path doesn't show this. What it reaches is getIPs() and resolvesTo() called
directly, and that is exactly what get_ips_for_url() and url_resolves_to() in
Subs-Compat.php do.

Fix is small — take the name and the result into locals so neither depends on how the host
is spelled at the end. It's the first commit on #9537 if you want to lift it.

For what it's worth, my own branch had the same fault by a different route: I'd added
unset($this->ips) to parse(), and toUtf8() calls parse(), so the property was gone
by the time it was returned. Same round trip, same result.

Three smaller things still outstanding

• CurlFetcher::result() works out the last response as count() - 1 and reads it
  without looking. With nothing recorded that is index -1, and WebFetchApi::fetch()
  asks for result('success') on exactly the path where nothing is recorded, so every
  refused fetch warns twice on its way to returning false. SocketFetcher::result() already
  guards this — curl just didn't get the same treatment. Also on [3.0][Testing] Cover the SNI regression in the unit suite #9537.

• make_fetch_safe() is still removed. It is a 2.1 function and Subs-Compat.php
  exists to keep 2.1 names callable, so any mod calling it now fatals. Bringing it back
  deprecated — returning the URL unchanged when safe, null when not — keeps
  $url = make_fetch_safe($url); if ($url === null) working, and what comes back is now
  actually fetchable, which it wasn't before.

• FtpConnection::passive() no longer checks the address the server tells us to
  connect to next, and the replacement only covers FtpFetcher. The package manager and
  the maintenance tools build FtpConnection directly in twelve places and never go
  through FtpFetcher, and both create_file() and list_dir() fsockopen() straight to
  $this->pasv['ip'].

Also

I was wrong about one thing I nearly raised: protected static array $ips; with no default
is fine. PHP auto-initialises on offset-assignment, so self::$ips[$host] = [] does not
throw. Checked it rather than guessed.

And #9511 as it stands would have caught the previous revision's proxied() fatal on its
own — all seven existing proxiedProvider cases went down with ArgumentCountError. That
one's fixed here now, but it's a decent argument for landing the suite.

[Sesquipedalian]

FYI for the AI:

1. make_fetch_safe() never appeared in any released version of SMF 2.1 and never will, so it doesnt need to be preserved in Subs-Compat.php
2. It was a mistake to check for global IP addresses in FtpConnection::passive(), which is why that has been undone in this PR. When FtpConnection is used by the Package Manager, connecting to local IP addresses is commonly needed and intended. The check for global IP addresses only belongs in FtpFetcher, not FtpConnection.

[albertlast]

Both taken, and both checked rather than just accepted.

make_fetch_safe() — you're right, and my reasoning was bad. I looked for it on the
release-2.1 branch, found it, and treated that as "2.1 public API". It has never shipped:

v2.1.5   no make_fetch_safe
v2.1.6   no make_fetch_safe
v2.1.7   no make_fetch_safe

It went onto the branch on 2026-08-04 (806bfb612, "is_fetch_safe() → make_fetch_safe()"),
after v2.1.7. Nothing can be calling it, so nothing needs preserving. Withdrawn.

FtpConnection::passive() — also right, and the reason is the part I'd missed. I saw a
guard against non-global addresses come out and read it as a dropped SSRF check, which is
what it looks like in isolation. But FtpConnection is how the Package Manager reaches an
FTP server the admin nominated, and that is routinely on the LAN, so the check was breaking
the ordinary case rather than protecting it. FtpFetcher is the one that fetches from the
open web, and that is where it belongs. Withdrawn too.

I've put both into AGENTS.md as #9538, quoting you, since in each case the wrong
conclusion is the one that looks reasonable from inside the repo — the next agent through
here will reach it too otherwise. The generalisable half of the second one, as I understand
it: a class that fetches from the open web and a class that talks to infrastructure the
admin configured want opposite defaults, so work out which you are looking at before
arguing a guard back in.

That leaves only CurlFetcher::result() from my list, which is the small one —
count() - 1 is -1 when nothing was recorded, and WebFetchApi::fetch() reads
result('success') on exactly that path, so a refused fetch warns twice on its way to
returning false. SocketFetcher::result() already guards it.

#9537 carries that plus the getIPs() round-trip fix, and nothing else — I've rebuilt it
on this branch and dropped everything you've knocked back here. Its diff against this PR is
now just those two files.

[sbulen]

I just ran a test, & this installed clean, no errors, and in fact the Fetch SMF Files Scheduled Task ran clean, no errors.

admin_info_files appears to have been properly refreshed with a clean set of files. To test this, I modified the existing entries & ran Fetch SMF Files. Entries were updated with the proper, current entries.

Note to self: You cannot test this by truncating admin_info_files & rerunning. Bad things happen. It needs the old entries laying about...

[Sesquipedalian]

Note to self: You cannot test this by truncating admin_info_files & rerunning. Bad things happen. It needs the old entries laying about...

Hm. An empty admin_info_files table is something we should be able to recognize and recover from. Could you please open an issue about it, @sbulen, so that we remember to fix it in Alpha 6?