[2.x] feat: stop auth extensions having to fork core internals

[novacuum]

Fixes four places where an extension that authenticates against an external identity provider has to copy or replace core internals to do so.

Changes proposed in this pull request:

One commit per gap; they're independent and can be dropped individually.

1. Extend\Middleware silently mis-inserts when its anchor is gone (fix)

array_splice($existingMiddleware, array_search($originalMiddleware, $existingMiddleware), 0, $newMiddleware);

array_search() returns false when the anchor was already replaced or removed by an earlier extender, and array_splice() reads false as offset 0. So insertAfter(AuthenticateWithSession::class, …) lands the middleware at the very front of the stack — ahead of HandleErrors, where nothing it throws can be reported — and nothing says so.

This is reachable with two ordinary extensions: one replace()s a middleware, the other insertAfter()s the same class, and whichever loads second gets moved to position 0. Insertions now throw with the frontend, the anchor and the middleware named. remove() keeps its no-op semantics for an absent entry; only insertions need an anchor to exist. Comparison is also now strict.

2. No return-URL validator, in three different flavours (feat)

ResponseFactory::make() documents $returnTo as "must be validated by the caller before being passed here" and ships nothing to validate it with, while LogOutController and LogOutViewController each carry an identical private copy of a host allow-list check. Third-party OAuth controllers have filled the gap independently — one accepts relative paths only, another compares against a hard-coded host list — so there are currently three unrelated security models for the same parameter.

Flarum\Http\ReturnUrlValidator covers both shapes core accepts: validate()/sanitize() for an absolute URL checked against the forum host plus redirectDomains, which is what ?return= on the logout endpoints takes, and validatePath() for the same-origin relative path ResponseFactory expects. Both logout controllers delegate to it and keep sanitizeReturnUrl()/getAllowedRedirectDomains() as protected shims.

Behaviour is deliberately unchanged: validate() still rejects relative paths, because they carry no host to check and a browser resolves //host off-site.

3. The remember cookie ignores its token's lifetime (fix)

Rememberer::remember() hardcoded RememberAccessToken::rememberCookieLifeTime() regardless of the token it was handed, and that method resolved self::$lifetime rather than static::$lifetime. A subclass declaring its own $lifetime therefore changed when the token stopped being valid but not when the cookie expired, so the browser kept presenting a dead token for up to the five-year default. Two bugs pointing the same way, and between them a documented-looking seam that silently does nothing.

The lifetime now comes off the token instance's class and resolves late. Relevant to any login that inherits session_remember from an external authority whose own session is nowhere near five years long — which is every OAuth login today, since ResponseFactory mints a RememberAccessToken for all of them.

4. AuthenticateWithSession::getActor() is private (feat)

An extension that needs to check a resolved actor against an external authority — an identity provider that may have revoked the upstream session since this one was established — has to replace() the whole middleware and reimplement the body, including the $session->invalidate() / regenerateToken() path taken when a token is no longer valid. That copy then diverges from core silently on every change to it. Now protected, so such an extension overrides it, calls parent::getActor(), and acts on the result.

Reviewers should focus on:

• The throw in (1) is a behaviour change on upgrade. An extension that mis-inserts today does so silently; after this it fails at boot. That is the point, but it converts a quiet misordering into a hard failure for a forum that currently "works". The alternatives are logging and appending, or logging and skipping — both leave the extension's middleware somewhere it didn't ask for. Happy to switch if you'd rather not break a boot during RC.
• Two of the four commits are additive, and I'm aware 2.x is bug-fix-only until GA. (1) and (3) are fixes; (2) and (4) add surface. Say the word and I'll pull the additive pair onto a separate PR for 2.1 and leave the two fixes here.
• LogOutController and LogOutViewController constructor signatures change — Config out, ReturnUrlValidator in, since Config was only there for the allow-list. Both are container-resolved and nothing in the monorepo instantiates them directly, and the protected methods still work for subclasses, but it is a signature change.
• Whether ResponseFactory::make() should validate $returnTo itself rather than documenting that its caller must. I left the contract as-is because existing OAuth extensions already validate before calling and double-validation would silently narrow what they accept, but "core hands you a documented obligation and no tool" is arguably the wrong default and this PR only fixes the missing tool.
• Whether the logout endpoints should accept a relative ?return=. They reject one today — validate() preserves that — but a same-origin path is the obvious common case and is strictly safer than the absolute URLs they do accept. Out of scope here; flagging it because the new validatePath() makes it a two-line change if you want it.

Screenshot

Not applicable — no user-facing or visual change.

Necessity

• Has the problem that is being solved here been clearly explained? — four cases where core's own extension points either misbehave silently, don't exist, or exist and don't work. (1) and (3) are defects on their own terms, independent of who is extending what.
• If applicable, have various options for solving this problem been considered? — for (2) the alternative was leaving the duplicated private helpers alone and documenting the expected shape, which is what produced three incompatible implementations downstream. For (4) the alternative was a flarum.http.session_actor_resolvers tagged list; protected is the smaller change and can grow into one later if a second use case turns up. For (1) see the first bullet above.
• For core PRs, does this need to be in core, or could it be in an extension? — all four are in core classes: an extender, a middleware, Rememberer, and two controllers. None is reachable from an extension without replacing or copying the class.
• Are we willing to maintain this for years / potentially forever? — (1), (3) and (4) remove special cases rather than adding any. (2) adds one small class whose behaviour is already implemented twice in the same repo, so the maintenance surface is going down, not up.

Confirmed

• Frontend changes: tested on a local Flarum installation. — no frontend changes.
• Frontend changes: tests are green (run yarn test in js/). — no frontend changes.
• Frontend changes: tests have been added, or are not appropriate here. — no frontend changes.
• Backend changes: tests are green (run composer test). — partially: I ran the unit suite only (phpunit -c tests/phpunit.unit.xml), not the integration suite, which needs a database I don't have provisioned.
• Backend changes: tests have been added, or are not appropriate here. — 20 unit tests across three new files, listed below.
• Where applicable, changes are suitable for all supported database drivers (MySQL, MariaDB, PostgreSQL, SQLite). — no schema, no migration, no queries touched.
• Core developer confirmed locally this works as intended.
• The description above is written by me and describes what this pull request actually does.

On the tests

Three new unit files, 20 tests:

• tests/unit/Extend/MiddlewareTest.php (6) — insertBefore/insertAfter placement, the throw for a missing anchor in both directions, the two-extender case where one replace()s the anchor the other inserts against, and that remove() is still a no-op for an absent entry. Core had no unit coverage of this extender; the integration extenders/MiddlewareTest covers placement through a real request but not the missing-anchor path.
• tests/unit/Http/ReturnUrlValidatorTest.php (12) — the allow list, acceptance and rejection for both contracts, unparseable and empty input, the explicit fallbacks, and a CRLF payload against validatePath().
• tests/unit/Http/RemembererTest.php (2) — the default lifetime and a subclass lifetime, which is the one that fails before this change.

Required changes:

• Related documentation PR: ReturnUrlValidator should be mentioned wherever $returnTo is documented for OAuth authors — the 2.0 upgrade guide currently tells them to validate it without saying with what. Happy to open that once the shape here is settled. While checking, I also noticed redirectDomains isn't documented in config.md at all, and it's the allow list this validator reads.