Repository navigation
Commit bcd9a2b
authored
fix(chrome): fall back to favicon shield when template.logo points at a missing file (#1480)
* fix(chrome): fall back to favicon shield when template.logo points at 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.
* fix(chrome): address adversarial review findings on template.logo fallback
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.1 parent 476f4b1 commit bcd9a2b
13 files changed
Lines changed: 1342 additions & 23 deletions
File tree
- web
- includes/View
- pages
- core
- tests
- e2e/specs/flows
- integration
- themes/default
- core
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
425 | 425 | | |
426 | 426 | | |
427 | 427 | | |
| 428 | + | |
428 | 429 | | |
429 | 430 | | |
430 | 431 | | |
| |||
4894 | 4895 | | |
4895 | 4896 | | |
4896 | 4897 | | |
| 4898 | + | |
4897 | 4899 | | |
4898 | 4900 | | |
4899 | 4901 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
37 | 59 | | |
38 | 60 | | |
39 | 61 | | |
| |||
0 commit comments