Skip to content

fix(theme): keep one session store for the selected theme #278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mambax7 merged 2 commits into from
Oct 21, 2026
Merged

fix(theme): keep one session store for the selected theme #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25011c0
fix(theme): keep one session store for the selected theme
mambax7 Oct 20, 2026
3ad6504
fix(theme): drop xoops_theme_select from the selector redirect and na…
mambax7 Oct 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/changelog.270.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
===================================
Expand Down
87 changes: 53 additions & 34 deletions htdocs/class/theme.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
}
if ($req !== '' && $this->isThemeAllowed($req)) {
$options['folderName'] = $req;
if (isset($_SESSION) && $this->allowUserSelection) {
$_SESSION['xoopsUserTheme'] = $req;

@sourcery-ai sourcery-ai Bot Oct 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium · Concurrent theme choices are lost

When two requests for the same session select different themes concurrently, a page request selecting alpha and a selector request selecting beta can both read the same session snapshot. The custom session handler writes the entire serialized session without locking, so whichever request saves last overwrites the other choice; later pages can show the losing theme. Add per-session locking or conflict-safe updates for the shared theme value.

Serialize session reads and writes with a per-session lock, or merge theme updates without overwriting newer session state.

Prompt for AI agents 
In `htdocs/class/theme.php` at line 81:

**Concurrent theme choices are lost**

When two requests for the same session select different themes concurrently, a page request selecting alpha and a selector request selecting beta can both read the same session snapshot. The custom session handler writes the entire serialized session without locking, so whichever request saves last overwrites the other choice; later pages can show the losing theme. Add per-session locking or conflict-safe updates for the shared theme value.

Serialize session reads and writes with a per-session lock, or merge theme updates without overwriting newer session state.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
} elseif (isset($_SESSION['xoopsUserTheme'])) {

@sourcery-ai sourcery-ai Bot Oct 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium · Remembered themes are lost

When a returning session has its selected theme only under the pre-upgrade factory key, resolveFolderName() checks only xoopsUserTheme and falls back to defaultTheme, so returning users lose their remembered selection after upgrade.

When xoopsUserTheme is absent, read and validate the legacy factory defaultTheme session value before falling back to the configured default.

Also at htdocs/class/theme.php:91.

Prompt for AI agents 
In `htdocs/class/theme.php` at line 83:

**Remembered themes are lost**

When a returning session has its selected theme only under the pre-upgrade factory key, `resolveFolderName()` checks only `xoopsUserTheme` and falls back to `defaultTheme`, so returning users lose their remembered selection after upgrade.

When `xoopsUserTheme` is absent, read and validate the legacy factory `defaultTheme` session value before falling back to the configured default.

Also at `htdocs/class/theme.php:91`.

// 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
Expand Down
21 changes: 20 additions & 1 deletion htdocs/class/xoopskernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down
4 changes: 4 additions & 0 deletions htdocs/include/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
],
Expand Down
31 changes: 31 additions & 0 deletions tests/unit/htdocs/class/XoopsKernelThemeRedirectTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,35 @@ public function testThemeRedirectUrlReadsPostValue(): void

$this->assertSame('/modules/newbb/viewpost.php?status=new', $kernel->themeRedirectUrl());
}

/**
* @return array<string, array{string, string}>
*/
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());
}
}
Loading