Skip to content

Commit 65497eb

Browse files
authored
fix: show default theme author/version on Settings > Themes (#1466) (#1470)
* fix: parse single-quoted theme.conf.php metadata on Settings > Themes (#1466) The theme discovery regex only matched double-quoted define() values, so the default theme card showed "by Unknown · v?" despite valid manifest data. * fix: harden ThemeConf parser after adversarial review (#1466) - Discriminate single- vs double-quoted define() branches so empty "" does not read an unset capture (Settings fatal). - Support escaped apostrophes in single-quoted values. - Sanitize theme_link (http/https only) and screenshot filename (basename). - Expand ThemeConfParseTest; document picker vs API include in system.php, ARCHITECTURE.md, translating.md, and AGENTS.md. * chore: regenerate api-contract.js for system.sel_theme docblock (#1466)
1 parent a6dce0e commit 65497eb

8 files changed

Lines changed: 229 additions & 28 deletions

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4546,6 +4546,7 @@ contributions without contacting every contributor individually.
45464546
| Add a cross-repo JSON contract (vendored schema lock + reader + extractor parity test) | `web/includes/Telemetry/Schema1.php` is the reference shape (`payloadFieldNames(): list<string>` over a Draft-7 JSON Schema lock file). Pair with one PHPUnit extractor parity test (collect() vs. lock file in both directions). The schema lock file is the single source of truth — don't mirror the field list into a markdown doc paired with a separate parity test, that pattern was tried for telemetry and removed because the duplication paid for the drift risk it created. Sync via a manual `make sync-<subsystem>-schema` target — no scheduled auto-PR. See "Cross-repo JSON contracts" under Conventions. |
45474547
| Display a user's own permission flags grouped by category | `Sbpp\View\PermissionCatalog::groupedDisplayFromMask($mask)` (`web/includes/View/PermissionCatalog.php`). Adding a new flag to `web/configs/permissions/web.json` requires a paired entry in `WEB_CATEGORIES`; `PermissionCatalogTest` enforces it. |
45484548
| Live-preview Markdown in a settings textarea | `system.preview_intro_text` JSON action + `web/themes/default/page_admin_settings_settings.tpl` (`.dash-intro-editor` / `.dash-intro-preview`) |
4549+
| Regex-read `theme.conf.php` metadata for the admin Settings → Themes picker (without executing the manifest) | `Sbpp\Theme\ThemeConf` (`web/includes/Theme/ThemeConf.php`) — `parseDefine()` + `sanitizeLink()` / `sanitizeScreenshotFilename()`; wired from `web/pages/admin.settings.php` discovery loop (#1466). JSON theme preview (`api_system_sel_theme`) still `include`s the manifest. Regression: `web/tests/integration/ThemeConfParseTest.php`. |
45494550
| Build an empty-state surface (first-run vs filtered, primary/secondary CTAs) | `.empty-state` rules in `web/themes/default/css/theme.css` + reference shapes in `page_servers.tpl`, `page_dashboard.tpl`, `page_bans.tpl`, `page_comms.tpl`, `page_admin_audit.tpl`, `page_admin_bans_protests.tpl`, `page_admin_bans_submissions.tpl` |
45504551
| Subdivide an admin route into `?section=<slug>` URLs (servers, mods, groups, comms, settings, **admins**, **bans**) | `web/pages/admin.settings.php` is the long-standing reference; #1239 brought servers / mods / groups / comms onto the same shape; #1259 unified the chrome on the Settings-style vertical sidebar; #1275 brought admins (`admins` / `add-admin` / `overrides`) and bans (`add-ban` / `protests` / `submissions` / `import` / `group-ban`) onto the same shape, deleting the page-level ToC (`page_toc.tpl`) along the way so `?section=` is now the **only** sub-route nav contract. The shared partial is `web/themes/default/core/admin_sidebar.tpl` (parameterized on `tabs` / `active_tab` / `sidebar_id` / `sidebar_label`); `web/includes/View/AdminTabs.php` (`Sbpp\View\AdminTabs`) opens `<div class="admin-sidebar-shell">`, emits the `<aside>` + link list, opens `<div class="admin-sidebar-content">`, and the page handler closes both wrappers (`echo '</div></div>'`) AFTER `Renderer::render(...)`. Each `$sections` entry carries `slug` + `name` + `permission` + `url` + `icon` (Lucide name); the link emits `<a href="?p=admin&c=<page>&section=<slug>" data-testid="admin-tab-<slug>" aria-current="page">` — never `<button onclick="openTab(...)">` (the JS handler was deleted at #1123 D1). See "Sub-paged admin routes" in Conventions. |
45514552
| Render sub-views inside a Pattern A section (e.g. protests / submissions current-vs-archive) | `?view=<slug>` query param + a server-rendered `.chip-row` of real anchors (each carries `data-active="true|false"` + `aria-selected`). Reference: the protests / submissions chip rows in `web/pages/admin.bans.php` (`?section=protests&view=archive` / `?section=submissions&view=archive`). Pre-#1275 the chips called `Swap2ndPane()` — a `web/scripts/sourcebans.js` helper deleted at #1123 D1, leaving them dead — and the page rendered both views simultaneously. The new shape only renders the active view's data path; back/forward and link sharing both work. |

‎ARCHITECTURE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ web/
9797
│ ├── View/ Sbpp\View\* — typed Smarty view-model DTOs
9898
│ ├── View/Install/ Sbpp\View\Install\* — install-wizard step DTOs (#1332)
9999
│ ├── Markup/ Sbpp\Markup\IntroRenderer — admin Markdown -> safe HTML
100+
│ ├── Theme/ Sbpp\Theme\ThemeConf — regex-read theme.conf.php metadata for the admin Themes picker (#1466)
100101
│ ├── Servers/ Sbpp\Servers\{SourceQueryCache, RconStatusCache} — per-(ip, port) cache around the xPaw A2S probe (#1311) + per-sid cache around the RCON `status` command (#PLAYER_CTX_MENU)
101102
│ ├── Upload/ Sbpp\Upload\UploadHandler — shared file-upload handler (perm + CSRF + extension allowlist + filename sanitiser + popup chrome) for the demo / icon / mapimage popup pages
102103
│ ├── Mail/ Sbpp\Mail\{Mail,Mailer,EmailType} — Symfony Mailer wrapper + enum

‎docs/src/content/docs/customization/translating.md‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,26 @@ translations, and select your new theme in the panel's settings.
2121
the copy to something distinctive (e.g. `german`, `default-fr`).
2222

2323
2. **Edit `theme.conf.php`** in your new theme. Update the metadata
24-
so the panel's theme picker displays your theme correctly:
24+
so the panel's theme picker displays your theme correctly. The
25+
shipped default theme uses **single-quoted** string literals; double
26+
quotes work too. Use plain `define('key', 'value');` lines — the
27+
picker regex-reads the file without executing it, so concatenation
28+
and other PHP expressions are not reflected on the Settings → Themes
29+
cards (the JSON theme-preview API does execute the manifest).
2530

2631
```php
2732
<?php
28-
define('theme_name', "SourceBans++ Deutsch");
29-
define('theme_author', "Your name");
30-
define('theme_version', "1.0.0");
31-
define('theme_link', "https://your-site.example.com");
32-
define('theme_screenshot', "screenshot.jpg");
33-
?>
33+
define('theme_name', 'SourceBans++ Deutsch');
34+
define('theme_author', 'Your name');
35+
define('theme_version', '1.0.0');
36+
define('theme_link', 'https://your-site.example.com');
37+
define('theme_screenshot', 'screenshot.jpg');
3438
```
3539

40+
Leave `theme_link` empty if you have no homepage; use only the
41+
screenshot **filename** (not a path) — it must live in your theme
42+
directory next to the templates.
43+
3644
3. **Translate each `.tpl` file.** Open the `.tpl` files in your
3745
theme directory and replace the English copy with your
3846
translation. Don't touch the parts wrapped in `{...}`. Those

‎web/api/handlers/system.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,11 @@ function api_system_check_version(array $params): array
250250
];
251251
}
252252

253+
/**
254+
* Preview a theme manifest. Includes theme.conf.php so PHP expressions
255+
* in the file are honoured; the admin Settings → Themes grid uses
256+
* {@see \Sbpp\Theme\ThemeConf::parseDefine()} instead (no second define()).
257+
*/
253258
function api_system_sel_theme(array $params): array
254259
{
255260
$theme = rawurldecode((string)($params['theme'] ?? ''));

‎web/includes/Theme/ThemeConf.php‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
<?php
2+
// SourceBans++ (c) 2014-2026 SourceBans++ Dev Team
3+
// Licensed under the Elastic License 2.0.
4+
// See LICENSE.txt for the full license text and THIRD-PARTY-NOTICES.txt for attributions.
5+
6+
declare(strict_types=1);
7+
8+
namespace Sbpp\Theme;
9+
10+
/**
11+
* Read theme.conf.php metadata without executing the file (PHP cannot
12+
* define() the same constant twice in one request).
13+
*
14+
* The admin Settings → Themes picker uses this regex reader. JSON API
15+
* handlers ({@see api_system_sel_theme}) include the manifest and read
16+
* live constants — richer PHP expressions work there but not in the picker.
17+
*/
18+
final class ThemeConf
19+
{
20+
/**
21+
* Pluck a `define('<key>', "<value>")` or `define('<key>', '<value>')`
22+
* literal out of a theme.conf.php source string.
23+
*/
24+
public static function parseDefine(string $src, string $key, string $default): string
25+
{
26+
$pattern = '/define\(\s*\'' . preg_quote($key, '/') . '\'\s*,\s*(?:"([^"]*)"|\'((?:[^\'\\\\]|\\\\.)*)\')\s*\)\s*;/';
27+
if (preg_match($pattern, $src, $m) !== 1) {
28+
return $default;
29+
}
30+
31+
if (preg_match('/,\s*"/', $m[0]) === 1) {
32+
$value = $m[1];
33+
} else {
34+
$value = self::unescapeSingleQuoted($m[2] ?? '');
35+
}
36+
37+
return strip_tags($value);
38+
}
39+
40+
/**
41+
* Allow only http(s) homepage URLs for theme cards; empty is valid.
42+
*/
43+
public static function sanitizeLink(string $link): string
44+
{
45+
$link = trim(strip_tags($link));
46+
if ($link === '') {
47+
return '';
48+
}
49+
if (preg_match('#^https?://#i', $link) !== 1) {
50+
return '';
51+
}
52+
53+
return $link;
54+
}
55+
56+
/**
57+
* Screenshot filenames must stay inside the theme directory (basename only).
58+
*/
59+
public static function sanitizeScreenshotFilename(string $name, string $default): string
60+
{
61+
$name = trim(strip_tags($name));
62+
$base = basename(str_replace('\\', '/', $name));
63+
if ($base === '' || $base === '.' || $base === '..' || str_contains($base, '/')) {
64+
return $default;
65+
}
66+
67+
return $base;
68+
}
69+
70+
private static function unescapeSingleQuoted(string $value): string
71+
{
72+
return str_replace(['\\\\', "\\'"], ['\\', "'"], $value);
73+
}
74+
}

‎web/pages/admin.settings.php‎

Lines changed: 11 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use Sbpp\View\AdminFeaturesView;
1313
use Sbpp\View\AdminLogsView;
1414
use Sbpp\View\AdminSettingsView;
15+
use Sbpp\Theme\ThemeConf;
1516
use Sbpp\View\AdminThemesView;
1617
use Sbpp\View\Perms;
1718
use Sbpp\View\Renderer;
@@ -258,8 +259,8 @@
258259
* constant twice in one process, so the regex is the only way to
259260
* enumerate without resetting state). B18 keeps the regex-based
260261
* discovery and just enriches it with author / version / link /
261-
* screenshot — every theme.conf.php is expected to declare those four
262-
* constants.
262+
* screenshot — every theme.conf.php is expected to declare those five
263+
* define() keys (single- or double-quoted string literals; see #1466).
263264
*/
264265
$validThemes = [];
265266
$themesDir = opendir(SB_THEMES);
@@ -275,32 +276,21 @@
275276
$confSrc = (string) @file_get_contents($confPath);
276277
$validThemes[] = [
277278
'dir' => $filename,
278-
'name' => themeConfMatch($confSrc, 'theme_name', $filename),
279-
'author' => themeConfMatch($confSrc, 'theme_author', 'Unknown'),
280-
'version' => themeConfMatch($confSrc, 'theme_version', '?'),
281-
'link' => themeConfMatch($confSrc, 'theme_link', ''),
282-
'screenshot' => 'themes/' . $filename . '/' . themeConfMatch($confSrc, 'theme_screenshot', 'screenshot.jpg'),
279+
'name' => ThemeConf::parseDefine($confSrc, 'theme_name', $filename),
280+
'author' => ThemeConf::parseDefine($confSrc, 'theme_author', 'Unknown'),
281+
'version' => ThemeConf::parseDefine($confSrc, 'theme_version', '?'),
282+
'link' => ThemeConf::sanitizeLink(ThemeConf::parseDefine($confSrc, 'theme_link', '')),
283+
'screenshot' => 'themes/' . $filename . '/' . ThemeConf::sanitizeScreenshotFilename(
284+
ThemeConf::parseDefine($confSrc, 'theme_screenshot', 'screenshot.jpg'),
285+
'screenshot.jpg',
286+
),
283287
'active' => $filename === SB_THEME,
284288
];
285289
}
286290
closedir($themesDir);
287291
}
288292
usort($validThemes, fn(array $a, array $b): int => strcasecmp($a['name'], $b['name']));
289293

290-
/**
291-
* Pluck a `define('<key>', "<value>")` literal out of a theme.conf.php
292-
* source string. Mirrors the legacy regex shape (double-quoted only)
293-
* so existing theme manifests keep parsing identically.
294-
*/
295-
function themeConfMatch(string $src, string $key, string $default): string
296-
{
297-
$pattern = '/define\(\s*\'' . preg_quote($key, '/') . '\'\s*,\s*"([^"]*)"\s*\)\s*;/';
298-
if (preg_match($pattern, $src, $m) === 1) {
299-
return strip_tags($m[1]);
300-
}
301-
return $default;
302-
}
303-
304294
/**
305295
* Whether STEAMAPIKEY is set to a non-empty value at runtime. Wrapped
306296
* in a function so PHPStan can't narrow the constant value against its

‎web/scripts/api-contract.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,6 +577,10 @@
577577
* @typedef {Object} ApiSystemRehashAdminsResponse
578578
*/
579579
/**
580+
* Preview a theme manifest. Includes theme.conf.php so PHP expressions in the
581+
* file are honoured; the admin Settings → Themes grid uses {@see
582+
* \Sbpp\Theme\ThemeConf::parseDefine()} instead (no second define()).
583+
*
580584
* @typedef {Object} ApiSystemSelThemeRequest
581585
* @typedef {Object} ApiSystemSelThemeResponse
582586
*/
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Sbpp\Tests\Integration;
6+
7+
use PHPUnit\Framework\TestCase;
8+
use Sbpp\Theme\ThemeConf;
9+
10+
/**
11+
* Issue #1466: the admin Themes picker regex-reads theme.conf.php
12+
* without executing it. The shipped default manifest uses
13+
* single-quoted define() values; the pre-fix parser only matched
14+
* double-quoted literals, so cards showed "by Unknown · v?".
15+
*/
16+
final class ThemeConfParseTest extends TestCase
17+
{
18+
public function testDefaultThemeConfParsesSingleQuotedDefines(): void
19+
{
20+
$src = (string) file_get_contents(ROOT . 'themes/default/theme.conf.php');
21+
22+
$this->assertSame('SourceBans++ Default', ThemeConf::parseDefine($src, 'theme_name', ''));
23+
$this->assertSame('SourceBans++ Dev Team', ThemeConf::parseDefine($src, 'theme_author', 'Unknown'));
24+
$this->assertSame('2.0.0', ThemeConf::parseDefine($src, 'theme_version', '?'));
25+
$this->assertSame('https://github.com/sbpp/sourcebans-pp', ThemeConf::parseDefine($src, 'theme_link', ''));
26+
$this->assertSame('screenshot.jpg', ThemeConf::parseDefine($src, 'theme_screenshot', ''));
27+
}
28+
29+
public function testParseDefineStillAcceptsDoubleQuotedManifests(): void
30+
{
31+
$src = <<<'PHP'
32+
<?php
33+
define('theme_name', "Fork Theme");
34+
define('theme_author', "Example Author");
35+
define('theme_version', "1.2.3");
36+
define('theme_link', "https://example.com/theme");
37+
define('theme_screenshot', "preview.png");
38+
PHP;
39+
40+
$this->assertSame('Fork Theme', ThemeConf::parseDefine($src, 'theme_name', ''));
41+
$this->assertSame('Example Author', ThemeConf::parseDefine($src, 'theme_author', 'Unknown'));
42+
$this->assertSame('1.2.3', ThemeConf::parseDefine($src, 'theme_version', '?'));
43+
$this->assertSame('https://example.com/theme', ThemeConf::parseDefine($src, 'theme_link', 'missing'));
44+
$this->assertSame('preview.png', ThemeConf::parseDefine($src, 'theme_screenshot', ''));
45+
}
46+
47+
public function testEmptyDoubleQuotedValueDoesNotFatal(): void
48+
{
49+
$src = "<?php\ndefine('theme_link', \"\");\n";
50+
51+
$this->assertSame('', ThemeConf::parseDefine($src, 'theme_link', 'fallback'));
52+
}
53+
54+
public function testEmptySingleQuotedValue(): void
55+
{
56+
$src = "<?php\ndefine('theme_link', '');\n";
57+
58+
$this->assertSame('', ThemeConf::parseDefine($src, 'theme_link', 'fallback'));
59+
}
60+
61+
public function testEscapedApostropheInSingleQuotedValue(): void
62+
{
63+
$src = "<?php\ndefine('theme_author', 'Bob\\'s Fork');\n";
64+
65+
$this->assertSame("Bob's Fork", ThemeConf::parseDefine($src, 'theme_author', 'Unknown'));
66+
}
67+
68+
public function testDoubleQuotedKeyIsNotMatched(): void
69+
{
70+
$src = 'define("theme_name", "Wrong key shape");';
71+
72+
$this->assertSame('fallback', ThemeConf::parseDefine($src, 'theme_name', 'fallback'));
73+
}
74+
75+
public function testSanitizeLinkAllowsEmptyAndHttpUrls(): void
76+
{
77+
$this->assertSame('', ThemeConf::sanitizeLink(''));
78+
$this->assertSame('https://example.com', ThemeConf::sanitizeLink('https://example.com'));
79+
$this->assertSame('http://example.com/path', ThemeConf::sanitizeLink('http://example.com/path'));
80+
}
81+
82+
public function testSanitizeLinkRejectsNonHttpSchemes(): void
83+
{
84+
$this->assertSame('', ThemeConf::sanitizeLink('javascript:alert(1)'));
85+
$this->assertSame('', ThemeConf::sanitizeLink('file:///etc/passwd'));
86+
}
87+
88+
public function testSanitizeScreenshotFilenameStripsPaths(): void
89+
{
90+
$this->assertSame('shot.jpg', ThemeConf::sanitizeScreenshotFilename('shot.jpg', 'screenshot.jpg'));
91+
$this->assertSame('shot.jpg', ThemeConf::sanitizeScreenshotFilename('../../../shot.jpg', 'screenshot.jpg'));
92+
$this->assertSame('screenshot.jpg', ThemeConf::sanitizeScreenshotFilename('', 'screenshot.jpg'));
93+
$this->assertSame('screenshot.jpg', ThemeConf::sanitizeScreenshotFilename('..', 'screenshot.jpg'));
94+
}
95+
96+
public function testDefaultThemeDiscoveryRowMatchesManifest(): void
97+
{
98+
$filename = 'default';
99+
$confSrc = (string) file_get_contents(ROOT . 'themes/default/theme.conf.php');
100+
101+
$row = [
102+
'author' => ThemeConf::parseDefine($confSrc, 'theme_author', 'Unknown'),
103+
'version' => ThemeConf::parseDefine($confSrc, 'theme_version', '?'),
104+
'link' => ThemeConf::sanitizeLink(ThemeConf::parseDefine($confSrc, 'theme_link', '')),
105+
];
106+
107+
$this->assertSame('SourceBans++ Dev Team', $row['author']);
108+
$this->assertSame('2.0.0', $row['version']);
109+
$this->assertSame('https://github.com/sbpp/sourcebans-pp', $row['link']);
110+
$this->assertStringContainsString(
111+
'themes/' . $filename . '/',
112+
'themes/' . $filename . '/' . ThemeConf::sanitizeScreenshotFilename(
113+
ThemeConf::parseDefine($confSrc, 'theme_screenshot', 'screenshot.jpg'),
114+
'screenshot.jpg',
115+
),
116+
);
117+
}
118+
}

0 commit comments

Comments
 (0)