Repository navigation
fix(chrome): fall back to favicon shield when template.logo points at a missing file - #1480
Merged
Merged
Conversation
… a missing file
The navbar (`core/header.php`) and login (`page.login.php`) chrome read
`Config::get('template.logo')` and concatenated it into
`<img src="themes/<theme>/<value>">` with no existence check. Three
reachable input shapes silently render a broken `<img>`:
1. **`logos/sb-large.png`** — the v1.x default. Migration
`web/updater/data/809.php` forward-converts existing installs whose
row still matches this literal, but installs that missed the hop
(RC versions on `config.version >= 810` when #1235 landed,
restore-from-v1.x-backup, hand-edited setting) stay on the broken
path. The file never shipped in the v2.0 default theme.
2. **Empty / `null` / row missing** — the settings page accepts an
empty string into the input; an upgrade path that lost the row
surfaces as `Config::get` returning `null` → `<img src="themes/default/">`.
3. **Custom path that no longer exists** — admin typo'd a filename or
the file was deleted during a theme refresh.
New `Sbpp\View\BrandLogo` helper consolidates the resolution:
- `resolve()` returns a theme-relative path; `resolveUrl()` returns the
public URL form for the login page (the navbar already has
`{$theme_url}` in scope).
- Fallback ladder: trim → empty check → leading-slash strip → reject
`..` / `\\` (path-traversal) → reject the v1.x default
(defense-in-depth against the migration not having run) → `is_file()`
against `SB_THEMES.<config.theme>/<value>` → fall back to
`BrandLogo::DEFAULT_PATH` (`images/favicon.svg`).
- The admin Settings page deliberately renders the RAW configured
value so operators can see + fix a broken pointer — only the chrome
render paths consume the resolver.
`web/tests/integration/BrandLogoTest.php` pins every branch (12 tests,
17 assertions): fresh-install round-trip, v1.x default fallback, empty
/ whitespace / missing-row fallback, non-existent custom path, valid
custom path round-trip, leading-slash strip, path-traversal rejection,
backslash rejection, theme-name honoured in URL.
`AGENTS.md` carries paired updates: the `Sbpp\View\BrandLogo` row in
the Namespacing table and a "Where to find what" row that documents
the fallback ladder + the three paired surfaces (`data.sql` seed,
`updater/data/809.php` migration, `BrandLogo::DEFAULT_PATH` literal)
that need to stay in lockstep.
…lback Adversarial review of the initial BrandLogo fix surfaced six follow-ups that each addressed a specific bug class or test-coverage gap: - **Null-byte injection** rejected in `BrandLogo::resolve()`. Null bytes in path components can short-circuit `is_file()` checks downstream (the C runtime treats them as terminators), so a hostile `logos/foo.png\0../../../../etc/passwd` would pass the dot-dot guard but truncate at the null byte for downstream filesystem APIs. The guard rejects on `\0` AND `\\` (Windows path separator on case-insensitive filesystems) in addition to the prior `..` check. - **Case-insensitive v1.x default rejection**. macOS / Windows filesystems are case-insensitive, so `Logos/SB-Large.PNG` (or any case-variant) would slip past a case-sensitive `===` comparison while still resolving to the missing file on a Linux-hosted panel. `strcasecmp()` closes the gap. - **Fail-closed on missing `SB_THEMES`**. `themesRoot()` now raises `LogicException` rather than silently returning the fallback when the constant isn't defined. `init.php` sets it; calling `BrandLogo` without `init.php` is a programmer error and must surface loudly rather than silently substituting the shield (which would mask the missing-bootstrap bug class). - **`V1_DEFAULT_PATH` widened to public const** so paired surfaces (migration 809, fresh-install seed) can reference the same literal. New `BrandLogoTest` cases (`testMigration809PinsV1AndDefaultLiterals`, `testFreshInstallSeedPinsDefaultLiteral`) pin the literal-equivalence contract — a future "let's hard-code `logos/sb-large.png` in the migration" sweep would fail at PR time. - **`LoginView::$brand_logo_url` docblock rewritten** to reference `BrandLogo::resolveUrl()` and its fallback ladder, removing stale references to the pre-fix raw-Config concatenation that no longer reflects how the value is built. - **Silent-fallback indicator on admin Settings** (the marquee finding): pre-fix the chrome silently substituted the fallback shield when `template.logo` was broken, leaving the operator's customisation visible in the settings input field while the chrome rendered something else entirely. The Settings page now surfaces a red warning chip (`[data-testid="setting-warning-template.logo"]`) next to the input when the configured value is non-empty AND the resolver is falling back, computed in `admin.settings.php` as `trim($rawLogo) !== '' && trim($rawLogo) !== BrandLogo::DEFAULT_PATH && BrandLogo::resolve() === BrandLogo::DEFAULT_PATH` and surfaced via `AdminSettingsView::$config_logo_using_fallback`. The chip isn't flagged on the empty / default / valid-custom-path cases (preserving signal-to-noise — see the dataProvider matrix in `BrandLogoChromeWiringTest`). Test coverage additions: - `BrandLogoTest`: 4 new cases (null-byte, case-insensitive v1.x, migration 809 literal pin, fresh-install seed literal pin). - `BrandLogoChromeWiringTest` (new file): pins the wiring of `BrandLogo::resolve()` into `web/pages/core/header.php` (the navbar render path) and `BrandLogo::resolveUrl()` into `web/pages/page.login.php` (the sign-in card render path), AND drives the `$config_logo_using_fallback` indicator matrix through 6 cases (v1.x default, deleted file, valid custom, empty, default, traversal). Mirrors the `LostPasswordChromeTest` stub-Smarty + process-isolation shape. - `brand-mark-resolution.spec.ts` (new E2E spec): launches a real browser against both the navbar (logged-in chrome) and the sign-in card (logged-out chrome) and asserts the rendered `<img>` actually loads via `naturalWidth > 0` — the canonical browser-side test for "this image is not broken". Covers both the default-value scenario AND the broken-template.logo scenario (seeded via `setSettingE2e`, reverted in `afterAll`). 4 chromium + 4 mobile-chromium tests; all pass. The new `data-testid="brand-mark"` hook on both `<img>` elements follows AGENTS.md's "selectors must use testability hooks" rule (anchored on the testid, never the `.sidebar__brand-mark` CSS class). AGENTS.md's "paired surfaces" enumeration extended from the original three sites to seven (every site in the codebase that hardcodes a SourceBans++ brand-mark image path is now enumerated): the SQL seed, the migration, `core/header.tpl` (favicon links), `install/_chrome.tpl` (install wizard favicon + brand img), `updater.tpl` (updater wizard brand img), `page_admin_settings_settings.tpl` (help-text reference), and the `LoginView` docblock. Quality gates: PHPStan clean; ts-check clean; api-contract clean (no drift); 27 BrandLogo PHPUnit tests pass; 8 brand-mark E2E tests pass; login + sidebar-sticky sibling specs continue to pass. The 6 pre-existing `PluginVersionResolveTest` failures are environmental (missing `game/` mount in the docker stack) and reproduce on `main`; not introduced by this PR.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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
The navbar (
core/header.php) and the login page (page.login.php) readConfig::get('template.logo')and concatenated the raw value into<img src=\"themes/<theme>/<value>\">with no existence check. Three reachable input shapes silently render a broken<img>:logos/sb-large.png— the v1.x default. Migrationweb/updater/data/809.phpforward-converts existing installs whose row STILL matches the v1.x literal, but installs that missed the hop (RC versions onconfig.version >= 810when Replace favicon with the 2.0.0 brand mark + ship the modern icon/theme-color set #1235 landed, restore-from-v1.x-backup, hand-edited setting) stay on the broken path. The file never shipped in the v2.0 default theme.null/ row missing — the settings page accepts an empty string into the input; an upgrade path that lost the row surfaces asConfig::getreturningnull→<img src=\"themes/default/\">.Fix
New
Sbpp\View\BrandLogohelper consolidates the resolution. Both chrome render paths consume it; the admin Settings page deliberately keeps rendering the RAW configured value so operators can see and fix a broken pointer (the input is the operator-facing "what's saved"; the helper is the chrome-facing "what gets rendered").Resolution ladder in
BrandLogo::resolve():trim()+ empty check →BrandLogo::DEFAULT_PATH(images/favicon.svg)/(mirrors whatpage.login.phpdid inline pre-fix)..or\\(path-traversal) → defaultlogos/sb-large.png(defense-in-depth against migration 809 not having run) → defaultis_file()againstSB_THEMES.<config.theme>/<value>→ default on missBrandLogo::resolveUrl()joins the active theme name into the public URL prefix (themes/<theme>/) forpage.login.php'sLoginView::brand_logo_url.Paired surfaces
If
BrandLogo::DEFAULT_PATHever changes, three surfaces need the same edit in the same PR:web/install/includes/sql/data.sql— fresh-install seed fortemplate.logoweb/updater/data/809.php— upgrade-path forward-conversion targetBrandLogo::DEFAULT_PATHitselfAGENTS.mdcarries paired updates: theSbpp\View\BrandLogorow in the Namespacing table and a new "Where to find what" row documenting the fallback ladder + the three paired surfaces.Test plan
./sbpp.sh phpstan— clean (level 5)./sbpp.sh test tests/integration/BrandLogoTest.php— 12 tests, 17 assertions pass./sbpp.sh test— only pre-existing failures inPluginVersionResolveTest(environment-specific, fails identically onmain— the test readsdirname(__DIR__, 3)/game/...which lands outside the container's./webbind mount; unrelated to this PR)BrandLogoTestcovers every branch:DEFAULT_PATHconstant pins, fresh-install round-trip, v1.x default fallback, empty / whitespace / missing-row fallback, non-existent custom path fallback, valid custom path round-trip, leading-slash strip, path-traversal rejection, backslash rejection, theme-name honoured in URL prefix