diff --git a/docs/changelog.270.txt b/docs/changelog.270.txt index fb04233df..4cdd6d461 100644 --- a/docs/changelog.270.txt +++ b/docs/changelog.270.txt @@ -1,5 +1,13 @@ XOOPS 2.7.x Changelog (Language changes: see: /docs/lang_diff.txt) +=================================== +2.7.5 NOT RELEASED +=================================== +Bug fixes: +- the theme selector block works again after a theme was chosen with ?xoops_theme_select= in a URL: the theme factory kept its own session copy of that choice, which overrode every later selection, and now uses the same $_SESSION['xoopsUserTheme'] as the selector and login (class/theme.php) +- a theme picked in the selector block is no longer replaced on the next page when the current URL still contains ?xoops_theme_select=: the selector redirect drops that parameter (class/xoopskernel.php) +- the admin redirect page renders the default theme its template variables are built for, instead of the visitor's session theme (include/functions.php) + =================================== 2.7.4 2026-10 =================================== diff --git a/htdocs/class/theme.php b/htdocs/class/theme.php index 98ee1c51b..a6b465ce6 100644 --- a/htdocs/class/theme.php +++ b/htdocs/class/theme.php @@ -53,49 +53,68 @@ class xos_opal_ThemeFactory public $allowUserSelection = true; /** - * Instantiate the specified theme - * @param array $options - * @param array $initArgs - * @return null|xos_opal_Theme + * Pick the theme folder when the caller did not name one. + * + * Order: xoops_theme_select from POST, then GET (remembered in + * $_SESSION['xoopsUserTheme']), then the remembered session theme, + * then $this->defaultTheme. The session key is the one the kernel's + * theme selector and login also use, so there is a single store. + * + * @param array $options createInstance() options + * @return array $options with 'folderName' resolved */ - public function createInstance($options = [], $initArgs = []) + protected function resolveFolderName(array $options): array { - // Grab the theme folder from request vars if present. // Use === '' rather than empty() — a theme directory literally // named "0" is legitimate (legacy XOOPS supports all-numeric // theme names), and empty('0') === true would silently drop it. - if (!isset($options['folderName']) || $options['folderName'] === '') { - $req = xoops_validateThemeName(Request::getString('xoops_theme_select', '', 'POST')); - if ($req === '') { - $req = xoops_validateThemeName(Request::getString('xoops_theme_select', '', 'GET')); - } - if ($req !== '' && $this->isThemeAllowed($req)) { - $options['folderName'] = $req; - if (isset($_SESSION) && $this->allowUserSelection) { - $_SESSION[$this->xoBundleIdentifier]['defaultTheme'] = $req; - } - } elseif (isset($_SESSION[$this->xoBundleIdentifier]['defaultTheme'])) { - // Stale session theme names can outlive an admin change - // to the allowed list; validate AND check membership - // before reaching the path-construction code below. - // xoops_validateThemeValue() — scalar-gated; a session - // poisoned with an array / object never reaches the - // (string) cast (which would emit a warning AND let - // "Array" pass the path-safety check downstream). - $sessionTheme = xoops_validateThemeValue($_SESSION[$this->xoBundleIdentifier]['defaultTheme']); - $options['folderName'] = ($sessionTheme !== '' && $this->isThemeAllowed($sessionTheme)) - ? $sessionTheme - : $this->defaultTheme; - } elseif (!isset($options['folderName']) || $options['folderName'] === '' || !$this->isThemeAllowed($options['folderName'])) { - $options['folderName'] = $this->defaultTheme; + if (isset($options['folderName']) && $options['folderName'] !== '') { + return $options; + } + $req = xoops_validateThemeName(Request::getString('xoops_theme_select', '', 'POST')); + if ($req === '') { + $req = xoops_validateThemeName(Request::getString('xoops_theme_select', '', 'GET')); + } + if ($req !== '' && $this->isThemeAllowed($req)) { + $options['folderName'] = $req; + if (isset($_SESSION) && $this->allowUserSelection) { + $_SESSION['xoopsUserTheme'] = $req; } + } elseif (isset($_SESSION['xoopsUserTheme'])) { + // Stale session theme names can outlive an admin change + // to the allowed list; validate AND check membership + // before reaching the path-construction code below. + // xoops_validateThemeValue() — scalar-gated; a session + // poisoned with an array / object never reaches the + // (string) cast (which would emit a warning AND let + // "Array" pass the path-safety check downstream). + $sessionTheme = xoops_validateThemeValue($_SESSION['xoopsUserTheme']); + $options['folderName'] = ($sessionTheme !== '' && $this->isThemeAllowed($sessionTheme)) + ? $sessionTheme + : $this->defaultTheme; + } else { + $options['folderName'] = $this->defaultTheme; } + + return $options; + } + + /** + * Instantiate the specified theme + * @param array $options + * @param array $initArgs + * @return null|xos_opal_Theme + */ + public function createInstance($options = [], $initArgs = []) + { + $options = $this->resolveFolderName($options); // Validate $options['folderName'] AND the defaultTheme property - // unconditionally, after every resolution branch. Two paths - // bypass the inner-if block above and could still produce an - // unsafe path segment: + // unconditionally, after every resolution branch. Two inputs + // reach this point without the request/session validation in + // resolveFolderName() and could still produce an unsafe path + // segment: // - A non-standard caller may have supplied a poisoned - // folderName directly (skipping the empty()-branch). + // folderName directly (returned unchanged). // - $this->defaultTheme is a public property; any caller // can overwrite it before createInstance() runs. // xoops_validateThemeValue() — scalar gate + path/HTML validate diff --git a/htdocs/class/xoopskernel.php b/htdocs/class/xoopskernel.php index 7fc339dc7..7a05b4a1f 100644 --- a/htdocs/class/xoopskernel.php +++ b/htdocs/class/xoopskernel.php @@ -224,14 +224,33 @@ public function themeSelect() /** * Return a safe same-site target for the theme selector redirect. * + * Any xoops_theme_select query pair is dropped: the selector posts the + * current page URL, and a ?xoops_theme_select= still in it would override + * the theme just chosen on the redirected GET. + * * @return string Validated redirect URL, or '' if no safe target was supplied. */ protected function getThemeRedirectUrl(): string { - return $this->validateThemeRedirectUrl( + $url = $this->validateThemeRedirectUrl( \Xmf\Request::getString('xoops_theme_redirect', '', 'POST'), XOOPS_URL ); + $parts = explode('#', $url, 2); + $pathQuery = explode('?', $parts[0], 2); + if (isset($pathQuery[1])) { + // parse_str() per pair names each key exactly as the redirected + // GET will ('.', '+', %5F, leading spaces, [] and an unclosed '['). + // Only the pair is parsed; the kept pairs are re-joined verbatim. + $kept = array_filter(explode('&', $pathQuery[1]), static function (string $pair): bool { + parse_str($pair, $parsed); + + return !array_key_exists('xoops_theme_select', $parsed); + }); + $parts[0] = $pathQuery[0] . ($kept === [] ? '' : '?' . implode('&', $kept)); + } + + return implode('#', $parts); } /** diff --git a/htdocs/include/functions.php b/htdocs/include/functions.php index f06a323f3..6d0f4ccf1 100644 --- a/htdocs/include/functions.php +++ b/htdocs/include/functions.php @@ -830,8 +830,12 @@ function redirect_header($url, $time = 3, $message = '', $addredirect = true, $a $xoopsThemeFactory = new xos_opal_ThemeFactory(); $xoopsThemeFactory->allowedThemes = $xoopsConfig['theme_set_allowed']; $xoopsThemeFactory->defaultTheme = $theme; + // Name the folder: the factory would otherwise prefer the visitor's + // session theme over defaultTheme, while the template variables below + // are built for $theme. $xoTheme = $xoopsThemeFactory->createInstance( [ + 'folderName' => $theme, 'plugins' => [], 'renderBanner' => false, ], diff --git a/tests/unit/htdocs/class/XoopsKernelThemeRedirectTest.php b/tests/unit/htdocs/class/XoopsKernelThemeRedirectTest.php index 87df7d43b..10aaebb63 100644 --- a/tests/unit/htdocs/class/XoopsKernelThemeRedirectTest.php +++ b/tests/unit/htdocs/class/XoopsKernelThemeRedirectTest.php @@ -87,4 +87,35 @@ public function testThemeRedirectUrlReadsPostValue(): void $this->assertSame('/modules/newbb/viewpost.php?status=new', $kernel->themeRedirectUrl()); } + + /** + * @return array + */ + public static function themeSelectParamProvider(): array + { + return [ + 'only pair' => ['/index.php?xoops_theme_select=alpha', '/index.php'], + 'middle pair' => ['/x.php?a=1&xoops_theme_select=alpha&b=2#top', '/x.php?a=1&b=2#top'], + 'dot alias' => ['/x.php?xoops.theme.select=alpha&a=1', '/x.php?a=1'], + 'plus alias' => ['/x.php?xoops+theme+select=alpha', '/x.php'], + 'percent encoded' => ['/x.php?xoops%5Ftheme%5Fselect=alpha', '/x.php'], + 'array form' => ['/x.php?xoops_theme_select[]=alpha&a=1', '/x.php?a=1'], + 'leading space' => ['/x.php?+xoops_theme_select=alpha', '/x.php'], + 'unclosed bracket' => ['/x.php?xoops[theme_select=alpha', '/x.php'], + 'similar key kept' => ['/x.php?xoops_theme_selected=1', '/x.php?xoops_theme_selected=1'], + 'value only kept' => ['/x.php?q=xoops_theme_select', '/x.php?q=xoops_theme_select'], + 'fragment kept' => ['/x.php?a=1#xoops_theme_select=alpha', '/x.php?a=1#xoops_theme_select=alpha'], + 'rejected target' => ['https://example.com/?xoops_theme_select=alpha', ''], + ]; + } + + #[DataProvider('themeSelectParamProvider')] + public function testThemeRedirectUrlDropsThemeSelectParam(string $input, string $expected): void + { + $_POST['xoops_theme_redirect'] = $input; + + $kernel = new TestableThemeRedirectKernel(); + + $this->assertSame($expected, $kernel->themeRedirectUrl()); + } } diff --git a/tests/unit/htdocs/class/XoopsThemeFactoryTest.php b/tests/unit/htdocs/class/XoopsThemeFactoryTest.php new file mode 100644 index 000000000..a63b8e510 --- /dev/null +++ b/tests/unit/htdocs/class/XoopsThemeFactoryTest.php @@ -0,0 +1,229 @@ +resolveFolderName($options); + } +} + +/** + * Theme folder resolution in xos_opal_ThemeFactory::createInstance(). + * + * The factory, the kernel's theme selector (POST) and login all keep the + * user's theme in $_SESSION['xoopsUserTheme']. The factory used to keep a + * second copy under its own bundle key, so a theme chosen once with + * ?xoops_theme_select= overrode every later choice made in the selector. + * + * @category Xoops + * @package Core + * @author XOOPS Development Team + * @copyright (c) 2000-2026 XOOPS Project (https://xoops.org) + * @license GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html) + * @link https://xoops.org + */ +#[CoversClass(\xos_opal_ThemeFactory::class)] +final class XoopsThemeFactoryTest extends TestCase +{ + private array $savedPost; + private array $savedGet; + private ?array $savedSession; + + protected function setUp(): void + { + parent::setUp(); + $this->savedPost = $_POST; + $this->savedGet = $_GET; + $this->savedSession = $_SESSION ?? null; + $_POST = []; + $_GET = []; + $_SESSION = []; + } + + protected function tearDown(): void + { + $_POST = $this->savedPost; + $_GET = $this->savedGet; + if (null === $this->savedSession) { + unset($_SESSION); + } else { + $_SESSION = $this->savedSession; + } + parent::tearDown(); + } + + private function factory(): TestableThemeFactory + { + $factory = new TestableThemeFactory(); + $factory->allowedThemes = ['default', 'alpha', 'beta', '0']; + $factory->defaultTheme = 'default'; + + return $factory; + } + + #[Test] + public function explicitFolderNameIsKeptAndNothingIsRemembered(): void + { + $_GET['xoops_theme_select'] = 'alpha'; + + $options = $this->factory()->resolve(['folderName' => 'beta']); + + $this->assertSame('beta', $options['folderName']); + $this->assertArrayNotHasKey('xoopsUserTheme', $_SESSION); + } + + #[Test] + public function explicitZeroFolderNameIsKept(): void + { + $options = $this->factory()->resolve(['folderName' => '0']); + + $this->assertSame('0', $options['folderName']); + } + + #[Test] + public function postSelectionWinsAndIsRemembered(): void + { + $_POST['xoops_theme_select'] = 'alpha'; + $_GET['xoops_theme_select'] = 'beta'; + + $options = $this->factory()->resolve(); + + $this->assertSame('alpha', $options['folderName']); + $this->assertSame('alpha', $_SESSION['xoopsUserTheme']); + } + + #[Test] + public function getSelectionIsUsedAndRememberedInTheSharedSessionKey(): void + { + $_GET['xoops_theme_select'] = 'beta'; + + $options = $this->factory()->resolve(); + + $this->assertSame('beta', $options['folderName']); + $this->assertSame(['xoopsUserTheme' => 'beta'], $_SESSION); + } + + #[Test] + public function selectorChoiceAfterAGetSelectionIsNotOverridden(): void + { + // Request 1: theme chosen with ?xoops_theme_select=alpha + $_GET['xoops_theme_select'] = 'alpha'; + $this->factory()->resolve(); + + // Request 2: the kernel handles the selector POST and stores beta, + // then redirects; the follow-up page carries no selection. + $_GET = []; + $_SESSION['xoopsUserTheme'] = 'beta'; + + $options = $this->factory()->resolve(); + + $this->assertSame('beta', $options['folderName']); + } + + #[Test] + public function disallowedSelectionFallsBackToTheRememberedTheme(): void + { + $_SESSION['xoopsUserTheme'] = 'alpha'; + $_GET['xoops_theme_select'] = 'notallowed'; + + $options = $this->factory()->resolve(); + + $this->assertSame('alpha', $options['folderName']); + $this->assertSame('alpha', $_SESSION['xoopsUserTheme']); + } + + #[Test] + public function invalidSelectionNameIsIgnored(): void + { + $_GET['xoops_theme_select'] = '../alpha'; + + $options = $this->factory()->resolve(); + + $this->assertSame('default', $options['folderName']); + $this->assertArrayNotHasKey('xoopsUserTheme', $_SESSION); + } + + #[Test] + public function rememberedThemeNoLongerAllowedFallsBackToDefault(): void + { + $_SESSION['xoopsUserTheme'] = 'removed'; + + $options = $this->factory()->resolve(); + + $this->assertSame('default', $options['folderName']); + } + + #[Test] + public function poisonedRememberedThemeFallsBackToDefault(): void + { + $_SESSION['xoopsUserTheme'] = ['alpha']; + + $options = $this->factory()->resolve(); + + $this->assertSame('default', $options['folderName']); + } + + #[Test] + public function noSelectionAndNoRememberedThemeUsesDefault(): void + { + $options = $this->factory()->resolve(['folderName' => '']); + + $this->assertSame('default', $options['folderName']); + $this->assertArrayNotHasKey('xoopsUserTheme', $_SESSION); + } + + #[Test] + public function selectionIsNotRememberedWhenUserSelectionIsDisabled(): void + { + $factory = $this->factory(); + $factory->allowUserSelection = false; + $_GET['xoops_theme_select'] = 'alpha'; + + $options = $factory->resolve(); + + $this->assertSame('alpha', $options['folderName']); + $this->assertArrayNotHasKey('xoopsUserTheme', $_SESSION); + } + + #[Test] + public function selectionWithoutASessionIsUsedButNotRemembered(): void + { + unset($_SESSION); + $_GET['xoops_theme_select'] = 'alpha'; + + $options = $this->factory()->resolve(); + + $this->assertSame('alpha', $options['folderName']); + $this->assertFalse(isset($_SESSION)); + } + + #[Test] + public function emptyAllowedListAllowsAnyValidName(): void + { + $factory = new TestableThemeFactory(); + $_GET['xoops_theme_select'] = 'gamma'; + + $this->assertSame('gamma', $factory->resolve()['folderName']); + } +}