From 6e67efa31b12d85c5cff4e1d4dfa01ccbe776867 Mon Sep 17 00:00:00 2001 From: rumblefrog Date: Tue, 26 May 2026 13:15:12 -0400 Subject: [PATCH 1/3] fix: parse single-quoted theme.conf.php metadata on Settings > Themes (#1466) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The theme discovery regex only matched double-quoted define() values, so the default theme card showed "by Unknown · v?" despite valid manifest data. --- web/includes/Theme/ThemeConf.php | 31 ++++++++++++++ web/pages/admin.settings.php | 27 ++++-------- web/tests/integration/ThemeConfParseTest.php | 44 ++++++++++++++++++++ 3 files changed, 82 insertions(+), 20 deletions(-) create mode 100644 web/includes/Theme/ThemeConf.php create mode 100644 web/tests/integration/ThemeConfParseTest.php diff --git a/web/includes/Theme/ThemeConf.php b/web/includes/Theme/ThemeConf.php new file mode 100644 index 000000000..bd04579a7 --- /dev/null +++ b/web/includes/Theme/ThemeConf.php @@ -0,0 +1,31 @@ +', "")` or `define('', '')` + * literal out of a theme.conf.php source string. + */ + public static function parseDefine(string $src, string $key, string $default): string + { + $pattern = '/define\(\s*\'' . preg_quote($key, '/') . '\'\s*,\s*(?:"([^"]*)"|\'([^\']*)\')\s*\)\s*;/'; + if (preg_match($pattern, $src, $m) === 1) { + $value = $m[1] !== '' ? $m[1] : $m[2]; + + return strip_tags($value); + } + + return $default; + } +} diff --git a/web/pages/admin.settings.php b/web/pages/admin.settings.php index 19e8b5d1f..c2db5f7e1 100644 --- a/web/pages/admin.settings.php +++ b/web/pages/admin.settings.php @@ -12,6 +12,7 @@ use Sbpp\View\AdminFeaturesView; use Sbpp\View\AdminLogsView; use Sbpp\View\AdminSettingsView; +use Sbpp\Theme\ThemeConf; use Sbpp\View\AdminThemesView; use Sbpp\View\Perms; use Sbpp\View\Renderer; @@ -259,7 +260,7 @@ * enumerate without resetting state). B18 keeps the regex-based * discovery and just enriches it with author / version / link / * screenshot — every theme.conf.php is expected to declare those four - * constants. + * constants (single- or double-quoted string values; see #1466). */ $validThemes = []; $themesDir = opendir(SB_THEMES); @@ -275,11 +276,11 @@ $confSrc = (string) @file_get_contents($confPath); $validThemes[] = [ 'dir' => $filename, - 'name' => themeConfMatch($confSrc, 'theme_name', $filename), - 'author' => themeConfMatch($confSrc, 'theme_author', 'Unknown'), - 'version' => themeConfMatch($confSrc, 'theme_version', '?'), - 'link' => themeConfMatch($confSrc, 'theme_link', ''), - 'screenshot' => 'themes/' . $filename . '/' . themeConfMatch($confSrc, 'theme_screenshot', 'screenshot.jpg'), + 'name' => ThemeConf::parseDefine($confSrc, 'theme_name', $filename), + 'author' => ThemeConf::parseDefine($confSrc, 'theme_author', 'Unknown'), + 'version' => ThemeConf::parseDefine($confSrc, 'theme_version', '?'), + 'link' => ThemeConf::parseDefine($confSrc, 'theme_link', ''), + 'screenshot' => 'themes/' . $filename . '/' . ThemeConf::parseDefine($confSrc, 'theme_screenshot', 'screenshot.jpg'), 'active' => $filename === SB_THEME, ]; } @@ -287,20 +288,6 @@ } usort($validThemes, fn(array $a, array $b): int => strcasecmp($a['name'], $b['name'])); -/** - * Pluck a `define('', "")` literal out of a theme.conf.php - * source string. Mirrors the legacy regex shape (double-quoted only) - * so existing theme manifests keep parsing identically. - */ -function themeConfMatch(string $src, string $key, string $default): string -{ - $pattern = '/define\(\s*\'' . preg_quote($key, '/') . '\'\s*,\s*"([^"]*)"\s*\)\s*;/'; - if (preg_match($pattern, $src, $m) === 1) { - return strip_tags($m[1]); - } - return $default; -} - /** * Whether STEAMAPIKEY is set to a non-empty value at runtime. Wrapped * in a function so PHPStan can't narrow the constant value against its diff --git a/web/tests/integration/ThemeConfParseTest.php b/web/tests/integration/ThemeConfParseTest.php new file mode 100644 index 000000000..bcd1bf578 --- /dev/null +++ b/web/tests/integration/ThemeConfParseTest.php @@ -0,0 +1,44 @@ +assertSame('SourceBans++ Default', ThemeConf::parseDefine($src, 'theme_name', '')); + $this->assertSame('SourceBans++ Dev Team', ThemeConf::parseDefine($src, 'theme_author', 'Unknown')); + $this->assertSame('2.0.0', ThemeConf::parseDefine($src, 'theme_version', '?')); + $this->assertSame('https://github.com/sbpp/sourcebans-pp', ThemeConf::parseDefine($src, 'theme_link', '')); + $this->assertSame('screenshot.jpg', ThemeConf::parseDefine($src, 'theme_screenshot', '')); + } + + public function testParseDefineStillAcceptsDoubleQuotedManifests(): void + { + $src = <<<'PHP' +assertSame('Fork Theme', ThemeConf::parseDefine($src, 'theme_name', '')); + $this->assertSame('Example Author', ThemeConf::parseDefine($src, 'theme_author', 'Unknown')); + $this->assertSame('1.2.3', ThemeConf::parseDefine($src, 'theme_version', '?')); + } +} From 8b27994c320abea11325bc53ca14cbc652e561dd Mon Sep 17 00:00:00 2001 From: rumblefrog Date: Tue, 26 May 2026 13:21:16 -0400 Subject: [PATCH 2/3] 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. --- AGENTS.md | 1 + ARCHITECTURE.md | 1 + .../content/docs/customization/translating.md | 22 ++++-- web/api/handlers/system.php | 5 ++ web/includes/Theme/ThemeConf.php | 53 +++++++++++-- web/pages/admin.settings.php | 11 ++- web/tests/integration/ThemeConfParseTest.php | 74 +++++++++++++++++++ 7 files changed, 151 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 36b95d745..6420d2a61 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4546,6 +4546,7 @@ contributions without contacting every contributor individually. | Add a cross-repo JSON contract (vendored schema lock + reader + extractor parity test) | `web/includes/Telemetry/Schema1.php` is the reference shape (`payloadFieldNames(): list` 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--schema` target — no scheduled auto-PR. See "Cross-repo JSON contracts" under Conventions. | | 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. | | 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`) | +| 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`. | | 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` | | Subdivide an admin route into `?section=` 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 `
`, emits the `
'`) AFTER `Renderer::render(...)`. Each `$sections` entry carries `slug` + `name` + `permission` + `url` + `icon` (Lucide name); the link emits `` — never `