improve(core): consolidate theme-config normalisation across runtime readers#81
Merged
Merged
Conversation
…readers Single source of truth for theme_set / theme_set_allowed validation so every runtime reader (header, login, remember-me, theme factory, xoops_getcss, system theme block, kernel theme switcher) sees the same fail-empty contract. Closes XOOPS#45. Adds two helpers in include/theme_config.php (required transitively by include/functions.php): xoops_validateThemeName(string): string fail-empty path + HTML safety check (rejects path separators, null bytes, leading dot, .. segments, < > & " '); spaces and non-ASCII names pass through unchanged so legitimate "My Theme" and CJK theme directories keep working. xoops_resolveThemeConfig(array): array returns ['theme_set' => string, 'theme_set_allowed' => list<string>] with non-scalar entries dropped, pipe-string allowed lists split, current theme injected into allowed list when valid but absent, and 'default' as the universal fallback. include/common.php applies the resolver via array_replace immediately after the file+DB config merge, so plain reads of those keys in code loaded afterwards (header.php, site-closed.php, the editors, the CSS URL helpers, the smarty resource plugin) are safe-by-invariant. Decision-point callers go through the resolver explicitly even when the boundary normalise covers them, so intent is obvious at the call site and the code survives any future caller that hits the path without going through common.php (CLI entry, partial init): - htdocs/include/checklogin.php - htdocs/include/common.php remember-me branch - htdocs/include/site-closed.php - htdocs/header.php - htdocs/class/xoopsform/formselecttheme.php - htdocs/modules/system/blocks/system_blocks.php xoops_getConfigOption('theme_set'*) has its own cache layer in XoopsConfigHandler and is NOT covered by the boundary normalise. xos_kernel_Xoops2::themeSelect() now resolves at the call site and strict-compares membership; the session-stored theme name is validated before being trusted. theme.php hardening: - $_SESSION[xoBundleIdentifier]['defaultTheme'] in createInstance() was previously assigned to $options['folderName'] without validation or membership check, then used as a path segment in $testPath. Validate the session value AND require isThemeAllowed() before assigning. - The two mid-request writes to $GLOBALS['xoopsConfig']['theme_set'] are piped through xoops_validateThemeName() so the boundary invariant set by common.php holds for every downstream reader. - isThemeAllowed() now uses strict in_array(..., true) - a theme literally named "0" no longer matches every allowed entry. xoops_getcss() validates its $theme parameter and falls back via the resolver when empty, so a future caller passing raw input cannot reach the <link href=> attribute or filesystem checks unvalidated. The .githooks/pre-commit hook gains two new sniffs to prevent regression: loose in_array() on theme_set_allowed, and raw assignment of $xoopsConfig['theme_set'*] to theme-factory properties. Tests: tests/unit/htdocs/include/ThemeConfigTest.php covers the validation matrix (rejection cases, theme named "0", spaces, non-ASCII, pipe-string splitting, non-scalar filtering, current theme injection, corrupted-current fallback to default, deduping, non-array-non-string allowed entries). Refs: - PR XOOPS#44 (the block-level closure that this consolidates) - getVar() 'n'-format note in repo CLAUDE.md (used in common.php / checklogin.php remember-me branches to avoid rejecting "&" entity-escaped by the default 's' format)
…x/issue-45-theme-config-helpers
Reviewer's GuideIntroduces centralized, side‑effect‑free helpers for validating and resolving theme configuration, applies them at the config boundary and at all theme decision points, hardens session and request handling around theme selection, and adds tests and pre‑commit checks to prevent regressions in theme_set / theme_set_allowed handling. Sequence diagram for runtime theme selection and validationsequenceDiagram
actor User
participant Browser
participant XoopsKernel as xos_kernel_Xoops2
participant ThemeFactory as xos_opal_ThemeFactory
participant ThemeHelpers as theme_config_helpers
User->>Browser: Submit xoops_theme_select
Browser->>XoopsKernel: HTTP POST xoops_theme_select
XoopsKernel->>ThemeHelpers: xoops_resolveThemeConfig(theme_set, theme_set_allowed from xoops_getConfigOption)
ThemeHelpers-->>XoopsKernel: theme_set_allowed
XoopsKernel->>ThemeHelpers: xoops_validateThemeName(xoops_theme_select)
ThemeHelpers-->>XoopsKernel: validated themeSelect
alt [themeSelect !== '' && in_array(themeSelect, theme_set_allowed, true)]
XoopsKernel->>XoopsKernel: xoops_setConfigOption(theme_set, themeSelect)
XoopsKernel->>XoopsKernel: set $_SESSION[xoopsUserTheme]
else [fallback to session theme]
XoopsKernel->>ThemeHelpers: xoops_validateThemeName($_SESSION[xoopsUserTheme])
ThemeHelpers-->>XoopsKernel: validated sessionTheme
XoopsKernel->>XoopsKernel: in_array(sessionTheme, theme_set_allowed, true)
XoopsKernel->>XoopsKernel: xoops_setConfigOption(theme_set, sessionTheme)
end
XoopsKernel->>ThemeFactory: createInstance(folderName from xoopsConfig['theme_set'])
ThemeFactory->>ThemeHelpers: xoops_validateThemeName(options['folderName'])
ThemeHelpers-->>ThemeFactory: validated folderName
ThemeFactory-->>Browser: Render theme
Flow diagram for centralized theme configuration normalisationflowchart LR
subgraph ConfigSource
A[xoopsConfig raw]
B[xoops_getConfigOption]
end
A --> C[xoops_resolveThemeConfig]
C --> D[common.php
array_replace xoopsConfig]
D --> H[header.php
xos_opal_ThemeFactory]
D --> I[site-closed.php
xos_opal_ThemeFactory]
D --> J[checklogin.php]
D --> K[common.php
remember-me]
D --> L[formselecttheme.php]
D --> M[xoops_getcss]
D --> N[system_blocks.php
b_system_themes_show]
D --> O[theme.php
createInstance]
B --> C2[xoops_resolveThemeConfig]
C2 --> P[xoopskernel.php
themeSelect]
subgraph Validators
C
C2
Q[xoops_validateThemeName]
end
J --> Q
K --> Q
M --> Q
O --> Q
P --> Q
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughAdds scalar-gated theme validation and a resolver, normalizes theme settings at startup, replaces duplicated defensive validation across UI and runtime (login, kernel, factory, templates), adds unit tests, and extends pre-commit sniffs to prevent loose checks and direct raw-config assignments. ChangesTheme Configuration Validation and Runtime Consolidation
Sequence Diagram(s)sequenceDiagram
participant User
participant common.php
participant theme_config.php
participant checklogin.php
participant xoopskernel.php
User->>common.php: Load application/config
common.php->>theme_config.php: xoops_resolveThemeConfig(xoopsConfig)
theme_config.php-->>common.php: {theme_set, theme_set_allowed}
common.php->>common.php: Update xoopsConfig with normalized pair
User->>checklogin.php: Submit login with theme
checklogin.php->>theme_config.php: xoops_validateThemeName(posted_theme)
theme_config.php-->>checklogin.php: validated_name or ''
checklogin.php->>common.php: in_array(validated_name, resolved.allowed, true)
checklogin.php->>xoopskernel.php: store session theme if valid
User->>xoopskernel.php: Subsequent request with session theme
xoopskernel.php->>theme_config.php: xoops_validateThemeValue(session_theme)
theme_config.php-->>xoopskernel.php: validated_value or ''
xoopskernel.php->>common.php: in_array(validated_value, resolved.allowed, true)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #81 +/- ##
============================================
+ Coverage 18.13% 18.19% +0.05%
- Complexity 7854 7861 +7
============================================
Files 666 667 +1
Lines 43208 43259 +51
============================================
+ Hits 7837 7872 +35
- Misses 35371 35387 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes theme configuration validation and normalization by introducing helper functions in htdocs/include/theme_config.php and applying them across various files to ensure path and HTML safety. Feedback on the changes suggests optimizing xoops_getcss by directly accessing the already-normalized $GLOBALS['xoopsConfig']['theme_set'] to avoid redundant calls, and using is_scalar instead of is_string when resolving theme_set to prevent numeric theme names like 0 from being incorrectly rejected.
Comment on lines
+897
to
899
| if ($theme === '') { | ||
| $theme = xoops_resolveThemeConfig($GLOBALS['xoopsConfig'] ?? [])['theme_set']; | ||
| } |
There was a problem hiding this comment.
Since $GLOBALS['xoopsConfig'] is already normalized at the configuration-merge boundary in common.php (via xoops_resolveThemeConfig), calling xoops_resolveThemeConfig again here is redundant and introduces unnecessary performance overhead on every xoops_getcss call. Directly accessing the already-normalized theme_set key with a fallback is more efficient.
if ($theme === '') {
$theme = $GLOBALS['xoopsConfig']['theme_set'] ?? 'default';
}
Comment on lines
+120
to
+122
| $currentTheme = is_string($rawCurrentTheme) | ||
| ? xoops_validateThemeName($rawCurrentTheme) | ||
| : ''; |
There was a problem hiding this comment.
If theme_set is configured as an integer (e.g., 0 due to database driver casting or custom overrides), is_string($rawCurrentTheme) will evaluate to false, causing it to incorrectly fall back to 'default'. Using is_scalar and casting to a string ensures that numeric theme names (like 0) are correctly preserved and validated, matching the behavior of theme_set_allowed below.
$currentTheme = is_scalar($rawCurrentTheme)
? xoops_validateThemeName((string) $rawCurrentTheme)
: '';![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- You’re calling
xoops_resolveThemeConfig()in multiple hot paths (common.php boundary, header.php, site-closed.php, kernel themeSelect, formselecttheme, xoops_getcss); consider either memoizing the normalised pair or reusing the boundary-normalised$xoopsConfigto avoid recomputing the same normalization several times per request. - Common/bootstrap paths such as
include/common.php,class/xoopskernel.php, andclass/theme.phpnow depend onxoops_validateThemeName()/xoops_resolveThemeConfig(); it may be safer to requireinclude/theme_config.phpexplicitly in those entry points rather than relying on the transitive include frominclude/functions.phpto avoid edge cases where load order changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’re calling `xoops_resolveThemeConfig()` in multiple hot paths (common.php boundary, header.php, site-closed.php, kernel themeSelect, formselecttheme, xoops_getcss); consider either memoizing the normalised pair or reusing the boundary-normalised `$xoopsConfig` to avoid recomputing the same normalization several times per request.
- Common/bootstrap paths such as `include/common.php`, `class/xoopskernel.php`, and `class/theme.php` now depend on `xoops_validateThemeName()` / `xoops_resolveThemeConfig()`; it may be safer to require `include/theme_config.php` explicitly in those entry points rather than relying on the transitive include from `include/functions.php` to avoid edge cases where load order changes.
## Individual Comments
### Comment 1
<location path="htdocs/class/theme.php" line_range="88" />
<code_context>
+ // Pipe writes back to $GLOBALS['xoopsConfig'] through the
+ // validator so the boundary-normalised invariant set by
+ // common.php holds for every downstream reader.
+ $GLOBALS['xoopsConfig']['theme_set'] = xoops_validateThemeName((string) $options['folderName']) ?: 'default';
}
$testPath = isset($options['themesPath'])
</code_context>
<issue_to_address>
**issue (bug_risk):** The fallback to literal 'default' can desync $xoopsConfig['theme_set'] from the factory’s defaultTheme and the configured default theme.
If $options['folderName'] fails validation, this forces xoopsConfig['theme_set'] to the hard-coded 'default', even when the configured default theme (and $this->defaultTheme) is different. This can desync the global config from the theme actually in use and may point to a non-existent theme. Prefer using $this->defaultTheme (or the resolved theme from xoops_resolveThemeConfig) instead of the literal 'default' to stay aligned with admin configuration.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes runtime validation/normalization of theme_set and theme_set_allowed so all theme readers/decision points (bootstrap boundary, login/remember-me, theme switching, theme factory, xoops_getcss, and the System theme block) behave consistently with a “fail-empty then fall back to default” contract.
Changes:
- Add shared helpers
xoops_validateThemeName()andxoops_resolveThemeConfig()and require them viainclude/functions.php. - Normalize
theme_set/theme_set_allowedonce at theinclude/common.phpconfig-merge boundary and also at key decision-point call sites. - Add PHPUnit coverage for the validation/normalization contract and extend pre-commit checks to prevent regressions (loose
in_array/ raw config assignment patterns).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
htdocs/include/theme_config.php |
Introduces shared validation + resolution helpers for theme config. |
htdocs/include/common.php |
Boundary-normalizes theme config after file+DB config merge; tightens remember-me theme handling. |
htdocs/include/functions.php |
Requires the new helper file; hardens xoops_getcss() by validating the theme parameter. |
htdocs/include/checklogin.php |
Validates user theme (raw 'n' read) and uses strict membership checks. |
htdocs/include/site-closed.php |
Builds ThemeFactory config via resolver rather than raw $xoopsConfig reads. |
htdocs/header.php |
Routes ThemeFactory configuration through the resolver for consistent invariants. |
htdocs/class/theme.php |
Hardens request/session theme selection and makes isThemeAllowed() strict. |
htdocs/class/xoopsform/formselecttheme.php |
Populates theme select options from the resolved allowed list. |
htdocs/class/xoopskernel.php |
Normalizes config in the xoops_getConfigOption() cache path and validates POST/session themes with strict membership checks. |
htdocs/modules/system/blocks/system_blocks.php |
Switches the theme-switch block to use centralized helpers rather than local closures. |
.githooks/pre-commit |
Adds sniffs to prevent reintroducing loose in_array and raw ThemeFactory assignments from $xoopsConfig. |
tests/unit/htdocs/include/ThemeConfigTest.php |
Adds unit tests covering the validation matrix and resolver behavior. |
| // parameter and may be passed raw by future callers. Validate so a | ||
| // tainted value (path separator, null byte, ..) cannot reach the | ||
| // <link href=> attribute or filesystem checks below. | ||
| $theme = xoops_validateThemeName((string) $theme); |
| // Pipe writes back to $GLOBALS['xoopsConfig'] through the | ||
| // validator so the boundary-normalised invariant set by | ||
| // common.php holds for every downstream reader. | ||
| $GLOBALS['xoopsConfig']['theme_set'] = xoops_validateThemeName((string) $options['folderName']) ?: 'default'; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.githooks/pre-commit:
- Around line 127-133: The current grep in the hits assignment is too broad and
matches local variables like $allowedThemes; update the regex used to set hits
so it only matches property writes (object->allowedThemes or
object->defaultTheme) by requiring the literal '->' before the property name,
e.g. change the pattern in the hits assignment from
'(allowedThemes|defaultTheme)[[:space:]]*=[[:space:]]*\$xoopsConfig\[' to
something like
'->[[:space:]]*(allowedThemes|defaultTheme)[[:space:]]*=[[:space:]]*\$xoopsConfig\['
so only property assignments trigger the report and the hook no longer
false-fires on local variable assignments.
In `@htdocs/include/site-closed.php`:
- Around line 42-44: The closed-site template still uses raw $xoopsConfig values
after normalizing the theme; update the assignments that set xoops_theme,
xoops_imageurl, and xoops_themecss to use the resolved $themeConfig values
returned by xoops_resolveThemeConfig (and the same $themeConfig['theme_set'] /
related keys used to set $xoopsThemeFactory->defaultTheme and ->allowedThemes)
so the template consistently references the normalized theme and image/CSS paths
when rendering the closed-site page.
In `@htdocs/include/theme_config.php`:
- Around line 2-59: Prepend the standard XOOPS copyright header block to the top
of htdocs/include/theme_config.php (so the file begins with the required project
header), then move the existing helper-specific rationale block immediately
after that header without changing its content; ensure you don't alter the
helper text or credits and keep the function_exists() guard comments intact (the
helper comments starting "Side-effect-free theme-config helpers..." should
follow the standard XOOPS header).
In `@tests/unit/htdocs/include/ThemeConfigTest.php`:
- Around line 5-7: ThemeConfigTest uses PHPUnit 9.6-incompatible #[Test]
attributes and non-standard method names (validate..., resolve...) so tests are
not discovered; rename each test method in class ThemeConfigTest to start with
test (e.g., testValidateSomething, testResolveSomething) and replace attribute
annotations (#[CoversFunction], #[Test]) with equivalent docblock tags (`@covers`,
`@test`) above the methods to preserve coverage metadata and restore discovery
across PHPUnit versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef5367d5-b640-4b0e-b68e-22713ff11c65
📒 Files selected for processing (12)
.githooks/pre-commithtdocs/class/theme.phphtdocs/class/xoopsform/formselecttheme.phphtdocs/class/xoopskernel.phphtdocs/header.phphtdocs/include/checklogin.phphtdocs/include/common.phphtdocs/include/functions.phphtdocs/include/site-closed.phphtdocs/include/theme_config.phphtdocs/modules/system/blocks/system_blocks.phptests/unit/htdocs/include/ThemeConfigTest.php
Comment on lines
+2
to
+59
| /** | ||
| * Side-effect-free theme-config helpers. | ||
| * | ||
| * Single source of truth for normalising the two theme entries in | ||
| * $xoopsConfig: | ||
| * | ||
| * - xoops_validateThemeName() — fail-empty validation of one theme | ||
| * directory name (path + HTML safety) | ||
| * - xoops_resolveThemeConfig() — returns a normalised pair | ||
| * ['theme_set' => string, | ||
| * 'theme_set_allowed' => list<string>] | ||
| * | ||
| * Why this file exists: prior to issue #45, only the System theme-switch | ||
| * block (b_system_themes_show) normalised these values; every other | ||
| * runtime reader trusted the raw config entries. A corrupted xoops_config | ||
| * row (mid-upgrade state, direct override in xoopsconfig.php, manual | ||
| * edit) could feed unvalidated strings straight into <link href=> | ||
| * attributes, theme-factory folder paths, and in_array() membership | ||
| * checks. Consolidating the normalisation here lets common.php apply it | ||
| * once at the config-merge boundary and lets every decision-point caller | ||
| * (login, remember-me, theme selector, factory) reach the same answer. | ||
| * | ||
| * Validation contract: fail-empty. xoops_validateThemeName() returns '' | ||
| * for any rejected input — never raises, never sanitises by stripping. | ||
| * XOOPS theme discovery (XoopsLists::getDirListAsArray) and the theme | ||
| * factory accept arbitrary directory names including spaces and | ||
| * non-ASCII characters, so the validator does NOT enforce a | ||
| * conservative [A-Za-z0-9_.-] alphabet (which would silently drop | ||
| * legitimate `My Theme` / `テーマ` directories). Instead two safety | ||
| * classes are enforced: | ||
| * | ||
| * - Path safety: reject empty, leading dot, path separators (/, \), | ||
| * null bytes, and `..` appearing as a path segment. | ||
| * - HTML safety: reject the five HTML metacharacters (< > & " '). | ||
| * No legitimate theme directory contains those, and rejecting them | ||
| * at the boundary lets the option label and screenshot URL be | ||
| * embedded without per-renderer escaping logic. | ||
| * | ||
| * Each function is wrapped in a function_exists() guard so the file is | ||
| * safe to require_once multiple times across mixed load orders (the | ||
| * helpers are referenced from include/common.php before functions.php | ||
| * has finished loading on some preload paths). | ||
| * | ||
| * You may not change or alter any portion of this comment or credits | ||
| * of supporting developers from this source code or any supporting source code | ||
| * which is considered copyrighted (c) material of the original comment or credit authors. | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * | ||
| * @category kernel | ||
| * @package core | ||
| * @author XOOPS Development Team | ||
| * @copyright (c) 2000-2026 XOOPS Project (https://xoops.org) | ||
| * @license GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html) | ||
| * @link https://xoops.org | ||
| * @since 2.7.0 | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Restore the standard XOOPS header block at the top of the file.
This new file starts with a custom helper rationale block, so it does not begin with the required XOOPS copyright header block. Please move the project-standard header to the top and place the helper-specific commentary after it.
As per coding guidelines: Every source file begins with the XOOPS copyright header block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/include/theme_config.php` around lines 2 - 59, Prepend the standard
XOOPS copyright header block to the top of htdocs/include/theme_config.php (so
the file begins with the required project header), then move the existing
helper-specific rationale block immediately after that header without changing
its content; ensure you don't alter the helper text or credits and keep the
function_exists() guard comments intact (the helper comments starting
"Side-effect-free theme-config helpers..." should follow the standard XOOPS
header).
Comment on lines
+5
to
+7
| use PHPUnit\Framework\Attributes\CoversFunction; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Which PHPUnit versions support the #[Test]and#[CoversFunction] attributes, and are they available in PHPUnit 9.6?
💡 Result:
PHPUnit versions 10.0 and later support the #[Test] and #[CoversFunction] attributes [1][2]. These attributes were introduced to replace the legacy docblock annotations as part of the transition to using native PHP 8 attributes for metadata [1][2]. PHPUnit 9.6 does not support the #[Test] and #[CoversFunction] attributes [3][2]. In version 9.6, developers must continue using the corresponding docblock annotations (e.g., @test and @covers) [3][2]. The ability to use PHP 8 attributes for test metadata was a primary feature introduced starting with PHPUnit 10 [2].
Citations:
- 1: https://stackoverflow.com/questions/76538436/phpunit-10-coverage-with-attributes
- 2: Support PHP 8 attributes for adding metadata to test classes and test methods as well as tested code units sebastianbergmann/phpunit#4502
- 3: https://docs.phpunit.de/en/9.6/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/unit/htdocs/include/ThemeConfigTest.php"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "== $FILE (head) =="
head -n 120 "$FILE"
echo
echo "== $FILE (tail) =="
tail -n 120 "$FILE"
echo
echo "== public functions in $FILE =="
rg -n "public function" "$FILE" || true
echo
echo "== attribute usage in $FILE =="
rg -n "#\[" "$FILE" || trueRepository: XOOPS/XoopsCore27
Length of output: 9992
Fix PHPUnit 9.6 test discovery in tests/unit/htdocs/include/ThemeConfigTest.php
ThemeConfigTest relies on #[Test] + method names like validate.../resolve.... PHPUnit 9.6 doesn’t support these PHPUnit attribute annotations, so the tests won’t be discovered. Rename methods to test{MethodName} (and switch #[CoversFunction]/#[Test] to docblock @covers/@test if you want consistent coverage metadata across versions).
🔧 Minimal compatible pattern
-use PHPUnit\Framework\Attributes\CoversFunction;
-use PHPUnit\Framework\Attributes\Test;
use PHPUnit\Framework\TestCase;
@@
-#[CoversFunction('xoops_validateThemeName')]
-#[CoversFunction('xoops_resolveThemeConfig')]
+/**
+ * `@covers` ::xoops_validateThemeName
+ * `@covers` ::xoops_resolveThemeConfig
+ */
class ThemeConfigTest extends TestCase
{
- #[Test]
- public function validateRejectsEmptyString(): void
+ public function testValidateRejectsEmptyString(): void
{
$this->assertSame('', xoops_validateThemeName(''));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/htdocs/include/ThemeConfigTest.php` around lines 5 - 7,
ThemeConfigTest uses PHPUnit 9.6-incompatible #[Test] attributes and
non-standard method names (validate..., resolve...) so tests are not discovered;
rename each test method in class ThemeConfigTest to start with test (e.g.,
testValidateSomething, testResolveSomething) and replace attribute annotations
(#[CoversFunction], #[Test]) with equivalent docblock tags (`@covers`, `@test`)
above the methods to preserve coverage metadata and restore discovery across
PHPUnit versions.
Follow-up on bf19863 (XOOPS#81). Nine review findings, all in-scope for the same issue XOOPS#45 consolidation. Codex review (three rounds): 1. themeSelect() ignored file-config overrides. xos_kernel_Xoops2::themeSelect() resolved exclusively from xoops_getConfigOption(), which reads the XoopsConfigHandler cache. var/configs/xoopsconfig.php overrides never reach that cache, so a site overriding theme_set_allowed at the file level could see the theme switcher reject themes the rest of the runtime accepts. Prefer the already-merged $GLOBALS['xoopsConfig']; fall back to xoops_getConfigOption() only when the global is unset (partial-init paths that may run before common.php hydrates). 2. createInstance() could still use an unsafe path segment. The previous patch validated the write back to $GLOBALS['xoopsConfig']['theme_set'] but $testPath was built from the unvalidated $options['folderName']. Two paths skip the inner resolution block: - A non-standard caller can pass a poisoned folderName directly. - $this->defaultTheme is a public property; any caller can overwrite it before createInstance() runs. Add an unconditional validate-with-fallback after the resolution block, so $testPath cannot use a tainted segment regardless of how folderName reached this point. 3. xoops_getcss() emitted a conversion warning on non-scalar input. The unconditional (string) cast warned on array / object / resource under PHP 8.x. Skip the cast on non-scalar input — the helper reaches the boundary-normalised fallback in that case anyway. 4. xoops_resolveThemeConfig() handled scalar allowed but not scalar current. The allowed-list branch already used is_scalar(); the current-theme branch used is_string(), which silently rejected a legitimate int 0 (legacy xoops_config rows for the all-numeric theme directory "0"). Make the two paths consistent — is_scalar for both, with non-scalar (array / object / resource) still rejected. Adds a test that an int 0 theme_set survives as the string "0". 5. Pre-commit sniff 11 was too broad. The raw-assignment regex matched any variable named allowedThemes or defaultTheme, including local variables that are downstream of a legitimate resolver call (e.g. $allowedThemes = $themeConfig[…] reading a key that happens to be theme_set_allowed). Require the `->` property operator so only property writes trigger the report. Add `--` to the grep call so the leading `->` isn't parsed as an option flag. 6. Theme name "0" was dropped by Elvis fallbacks (R-063). The createInstance() validate-with-fallback added in (2) used `xoops_validateThemeName($x) ?: $fallback` and PHP treats the string '0' as falsy, so a perfectly valid all-numeric theme directory was silently replaced by $fallback. Two more spots had the same shape via empty(): - empty($options['folderName']) at the start of createInstance() treated '0' as missing and re-resolved it. - empty($theme_set) in XoopsTpl::setCompileId() swapped a '0' theme name for $xoopsConfig['theme_set'] when building the compile-id. Replace all four with explicit === '' checks (theme.php twice, setCompileId once, plus the inner-block elseif at line 82). The other empty() calls in setCompileId (template_set, module_dirname) are untouched — out of scope for issue XOOPS#45. 9. Unguarded (string) casts on potentially non-scalar values. Four call sites still cast session payloads, public properties, and raw $options entries to string before validation: - theme.php session-trust branch ($_SESSION[...]['defaultTheme']). - theme.php $this->defaultTheme. - theme.php $options['folderName']. - xoopskernel.php $_SESSION['xoopsUserTheme']. `(string) ['x']` emits a conversion Warning under PHP 8.x AND returns the literal string "Array", which then passes the path-safety check in xoops_validateThemeName() and reaches the theme path logic. Introduce a third helper xoops_validateThemeValue() in theme_config.php that bundles the is_scalar gate + xoops_validateThemeName() — the established pattern from xoops_getcss() and xoops_resolveThemeConfig() — and route every untrusted-source call site through it. xoops_resolveThemeConfig() current-theme branch and allowed-list array_map also collapse onto the new helper so the validation contract is identical everywhere. Adds tests covering scalar coercion (int, float, bool), non-scalar rejection (array, object, null), and path-safety pass-through on coerced values; smoke-test against a custom error handler confirms zero PHP warnings on any non-scalar input. CodeRabbit review: 7. site-closed.php template assigns used raw $xoopsConfig reads. The factory assignments above were updated to use the resolved $themeConfig; the template assigns below still read $xoopsConfig directly. They are safe-by-invariant from the boundary normalise, but reading the actual post-factory state matches the "theme not found" fallback in createInstance (which writes 'default' back to $GLOBALS['xoopsConfig']['theme_set'] when theme.tpl / theme.html is absent). Re-resolve after createInstance and use the post- factory pair for the template; the global is then read in exactly one place. Scrutinizer: 8. PHPDoc generics syntax. `list<string>` is a PHPStan/Psalm-only shape; Scrutinizer's PHPDoc parser raises "Documentation Bug" on it. Replace with `string[]` — universally parsed, PHPStan still accepts it as an alias for array<int|string, string>. The function still returns array_values(...) at runtime so the list shape is preserved at the value level even if the type shape is widened. Not addressed: - The PHPUnit 9.6 attribute claim. This repo runs PHPUnit ^11.2 (tests/phpunit.xml.dist line 3) and uses #[Test] / #[CoversClass] by convention. CI is green across PHP 8.2-8.5. - The XOOPS header-order point on theme_config.php. The file matches the established include/file_safety.php pattern (summary, rationale, BSD notice, @category/@package/@author/@copyright/@license/@link), so reordering would diverge from the in-tree convention. Refs: - PR XOOPS#81 review comments by Codex (three passes), CodeRabbit, Scrutinizer. - R-063 in base.md — Elvis-drops-zero, the rule violated and now re-fixed here.
Comment on lines
+5
to
+25
| use PHPUnit\Framework\Attributes\CoversFunction; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| require_once XOOPS_ROOT_PATH . '/include/theme_config.php'; | ||
|
|
||
| /** | ||
| * Coverage for xoops_validateThemeName() and xoops_resolveThemeConfig() | ||
| * in htdocs/include/theme_config.php. The helpers consolidate the | ||
| * defensive normalisation previously inlined in | ||
| * b_system_themes_show() so that every runtime reader of | ||
| * theme_set / theme_set_allowed shares one source of truth. | ||
| * | ||
| * Validation contract is fail-empty: invalid names return '', invalid | ||
| * config entries are dropped, current theme always survives in the | ||
| * allowed list, and a totally-corrupted config falls back to 'default'. | ||
| */ | ||
| #[CoversFunction('xoops_validateThemeName')] | ||
| #[CoversFunction('xoops_validateThemeValue')] | ||
| #[CoversFunction('xoops_resolveThemeConfig')] | ||
| class ThemeConfigTest extends TestCase |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.githooks/pre-commit (1)
118-125: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueLoose-
in_arraysniff:[^)]*makes it a leaky heuristic, but acceptable.The body class
[^)]*stops at the first), so a call likein_array(getTheme(), $cfg['theme_set_allowed'])won't match because ofgetTheme()'s closing paren. Likewise the strict-mode exclusion, true)is line-wide, so a second strictin_array(...)on the same line can mask a loose first one. Both are false-negatives, not false-positives — the hook won't block clean commits — so this is fine as a best-effort sniff. Worth a one-line note that multi-statement lines may slip through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.githooks/pre-commit around lines 118 - 125, The current loose in_array sniff (the hits variable using grep -E "in_array[[:space:]]*\([^)]*(theme_set_allowed)" and the strict exclusion grep -vE ",[[:space:]]*true[[:space:]]*\)") can miss calls when the argument contains nested parentheses or when multiple in_array calls share a line; add a concise one-line comment above the hits assignment explaining this limitation (that the regex stops at the first ')' and multi-statement lines can produce false-negatives) and reference the symbols in_array, theme_set_allowed, hits and report so future maintainers know why the heuristic is imperfect and why it remains acceptable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.githooks/pre-commit:
- Around line 118-125: The current loose in_array sniff (the hits variable using
grep -E "in_array[[:space:]]*\([^)]*(theme_set_allowed)" and the strict
exclusion grep -vE ",[[:space:]]*true[[:space:]]*\)") can miss calls when the
argument contains nested parentheses or when multiple in_array calls share a
line; add a concise one-line comment above the hits assignment explaining this
limitation (that the regex stops at the first ')' and multi-statement lines can
produce false-negatives) and reference the symbols in_array, theme_set_allowed,
hits and report so future maintainers know why the heuristic is imperfect and
why it remains acceptable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4f64f21-44e9-49c9-9b75-6d8c5075d7d5
📒 Files selected for processing (8)
.githooks/pre-commithtdocs/class/template.phphtdocs/class/theme.phphtdocs/class/xoopskernel.phphtdocs/include/functions.phphtdocs/include/site-closed.phphtdocs/include/theme_config.phptests/unit/htdocs/include/ThemeConfigTest.php
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
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.
Summary
Single source of truth for
theme_set/theme_set_allowedvalidation so every runtime reader — header render, login, remember-me, theme factory,xoops_getcss, the System theme block, and the kernel theme switcher — sees the same fail-empty contract.Closes #45.
Extends the closure-based defence PR #44 added to the System theme block into a process-wide guarantee, and plugs three real session-trust / path-construction leaks the issue's table didn't enumerate.
What this PR does
1. Two new helpers in
htdocs/include/theme_config.phpxoops_validateThemeName— fail-empty path + HTML safety check (rejects path separators, null bytes, leading dot,..segments,< > & " '). Spaces and non-ASCII names pass through unchanged so legitimateMy Themeand CJK theme directories keep working.xoops_resolveThemeConfig— returns['theme_set' => string, 'theme_set_allowed' => list<string>]with non-scalar entries dropped, pipe-string allowed lists split, current theme injected into the allowed list when valid but absent, and'default'as the universal fallback.Required transitively via
include/functions.php.2. Boundary normalise in
include/common.phpImmediately after the file+DB config merge,
$xoopsConfigisarray_replaced with the resolved pair. Plain reads of those keys in code loaded afterwards (header.php,site-closed.php, the editors, the CSS URL helpers, the smarty resource plugin) are then safe-by-invariant — no per-call-site sweep needed for cosmetic reads.3. Decision-point callers normalize explicitly
Six callers go through the resolver at the call site, even where the boundary normalise already covers them, so the intent is obvious and the code survives any future caller that hits the path without going through
common.php(CLI entry, partial init):htdocs/include/checklogin.phphtdocs/include/common.phpremember-me branchhtdocs/include/site-closed.phphtdocs/header.phphtdocs/class/xoopsform/formselecttheme.phphtdocs/modules/system/blocks/system_blocks.php4. Kernel theme switcher (separate cache layer)
xoops_getConfigOption('theme_set'*)reads throughXoopsConfigHandler's in-memory cache, which is not covered by the boundary normalise.xos_kernel_Xoops2::themeSelect()now:in_array(..., true)so a theme literally named0doesn't coerce to false against arbitrary allowed entries.5.
htdocs/class/theme.phphardening (three real bugs)$_SESSION[$this->xoBundleIdentifier]['defaultTheme']increateInstance()was previously assigned to$options['folderName']without any validation or membership check, then used as a path segment when constructing$testPath. A stale session value (admin removed the theme from the allowed list, manually-poisoned session) reachedfile_exists()unvalidated. Now validated AND required to passisThemeAllowed()before assignment.$GLOBALS['xoopsConfig']['theme_set']insidecreateInstance()are piped throughxoops_validateThemeName()so the boundary invariant set bycommon.phpholds for every downstream reader.in_array.isThemeAllowed()now usesin_array(..., true)— a theme literally named0no longer matches every allowed entry.6.
xoops_getcss()defence in depthValidates its
$themeparameter and falls back via the resolver when empty, so a future caller passing raw input cannot reach the<link href=>attribute or filesystem checks unvalidated.7. Pre-commit regression sniffs
Two new sniffs in
.githooks/pre-commit:in_array(..., $xoopsConfig['theme_set_allowed'])— strict comparison required.$xoopsThemeFactory->allowedThemes = $xoopsConfig['theme_set_allowed']— must go through the resolver.8. Tests
tests/unit/htdocs/include/ThemeConfigTest.phpcovers the full validation matrix:/,\, null byte, parent-dir segment, HTML metachars).0.theme_set_allowedsplitting.default.theme_set_allowedtreated as empty.Files changed (12)
New (2)
htdocs/include/theme_config.phptests/unit/htdocs/include/ThemeConfigTest.phpModified (10)
htdocs/include/common.phphtdocs/include/functions.phphtdocs/include/checklogin.phphtdocs/include/site-closed.phphtdocs/header.phphtdocs/class/theme.phphtdocs/class/xoopsform/formselecttheme.phphtdocs/class/xoopskernel.phphtdocs/modules/system/blocks/system_blocks.php.githooks/pre-commitOut of scope
Same exclusions the issue called out — admin-side theme-list editors (the saved-value path) and directory-name regex vs on-disk existence checks are not addressed here.
Test plan
ThemeConfigTestcovers all 22 cases listed above.default+ one custom theme; switch via the System block; log out / in; verifyxoopsUserThemeround-trips.xoops_config.theme_set_allowedwith a non-string row; visit any page — front + admin must still render with thedefaultfallback (no fatal, no warning).?xoops_theme_select=../etc/passwd— must be rejected at the factory, fall back to default.users.themevalue ofMy Theme(with space) — must round-trip throughchecklogin.phpandcommon.phpremember-me.bash .githooks/pre-commitagainst a synthetic staged change that re-introduces a loosein_arrayontheme_set_allowed— must reject.References
Summary by Sourcery
Centralize theme configuration normalization and harden theme selection across the runtime to enforce consistent, validated theme_set and theme_set_allowed usage.
Bug Fixes:
Enhancements:
Tests:
Chores:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores