Skip to content

Commit e0dfd0e

Browse files
authored
Fix sbpp#1335: post-install-wizard audit (panel-takeover paths + 8 UX gaps) (sbpp#1336)
* Fix sbpp#1335 C1 + M1: harden panel-runtime guard, swap bare die() for chrome C1 (critical, panel-takeover via localhost host-header bypass): pre-fix `web/init.php`'s install/ + updater/-presence guard was gated on `$_SERVER['HTTP_HOST'] != "localhost"`, so any panel reachable via a `localhost` Host header (port-forward, SSH tunnel, ngrok, Cloudflare Tunnel) silently bypassed the safety check. Combined with the absent wizard-side gate (sbpp#1335 C2, separate commit), an attacker could re-run the wizard over a live install and overwrite `config.php` / admin / DB. The exemption is gone; the guard is now unconditional in production, with a single explicit `SBPP_DEV_KEEP_INSTALL` constant as the dev escape hatch (loud-named so a production-side define is visibly wrong; defined automatically for the docker dev stack via `docker/php/dev-prepend.php`'s `auto_prepend_file` hook). The legacy `IS_UPDATE` exemption — used by the updater itself — is preserved. M1 (major, error chrome): the wizard's done-page CTA sends operators straight to `/`. Pre-fix, that landed on bare-text `die('SourceBans++ is not installed')` / `die('Please delete the install directory')` / `die('Composer autoload not found')` — stark white 200-response with no chrome / no link to docs / no fix instructions. Reads like a server crash to a non-technical self-hoster. Replaced with self-contained inline-HTML error pages in `web/init-recovery.php`'s `sbpp_render_install_blocked_page()` (mirror of `recovery.php`'s contract: no Composer / no Smarty / no `Sbpp\…`, since the surface runs upstream of the autoload chain). Bonus per the issue: missing-`config.php` now redirects to `/install/` instead of dying so a fresh-tarball operator lands directly in the wizard. Guard logic and rendering live in pure functions (`sbpp_check_install_guard()` + `sbpp_render_install_blocked_page()`) so the contract is unit-testable in isolation. `web/tests/integration/InstallGuardTest.php` pins: - localhost Host header does NOT bypass the guard (C1 regression) - IS_UPDATE skips the guard (preserved legacy contract) - SBPP_DEV_KEEP_INSTALL skips the guard (new dev escape hatch) - the C2 already-installed predicate (sister-guard, separate commit) - the m4 PDO error translation (separate commit) The dev stack: - `docker/php/dev-prepend.php` defines `SBPP_DEV_KEEP_INSTALL` (replaces the pre-sbpp#1335 `HTTP_HOST` rewrite trick — that whole shape was the vulnerability path) - `docker/Dockerfile` and `docker-compose.yml` comment-only updates document the new mechanism * Fix sbpp#1335 C2: refuse wizard start over already-installed panel Pre-fix, `web/install/index.php` had no "is the panel already installed?" gate. Anyone reaching `/install/` after a successful wizard run (operator forgot to delete `install/`, or guard bypass via sbpp#1335 C1's localhost host-header trick) could walk the wizard end-to-end again, overwriting `config.php` (when writable), creating a new admin account, and re-pointing the panel at a different DB — a complete panel-takeover path. The wizard now refuses to start when `config.php` exists in the panel root, surfacing `web/install/already-installed.php` (pure inline HTML + CSS, mirror of `recovery.php`'s contract: no Composer / no Smarty / no `Sbpp\…` since the gate runs BEFORE `bootstrap.php`'s autoload pull). The page emits 409 Conflict, links the operator back to `/` (already-installed panels boot from there), and explains the intentional-reinstall path: delete `config.php` first. No confirm-dialog bypass is offered — the explicit delete step forces the operator to acknowledge the impact before the wizard touches any state. The guard predicate is a pure function (`sbpp_install_is_already_installed()`) so the contract is unit-testable without a runtime install; the regression test lives in `web/tests/integration/InstallGuardTest.php` (added in the previous commit alongside the C1 / M1 / m4 coverage). Sister-guard to the runtime-side `sbpp_check_install_guard()` from the previous commit; both key off `config.php` so the contract is symmetric. * Fix sbpp#1335 M2 + M3 + m1-m6: install wizard human-flow polish Wizard-side UX fixes from the sbpp#1335 audit, grouped into one commit because every change is a localized template / handler tweak that compounds the same goal: stop the wizard from feeling broken when something goes wrong on the operator's first touch. M2 (writable folder fail message): page 3 surfaced "Not writable: /path/..." with no remediation. Now extends with a one-liner pointing operators at chmod 0775 (or 0777 on shared hosting where the PHP user isn't yours) via File Manager / FTP / chmod. Plain text, not HTML — the surrounding Smarty template auto-escapes. M3 (admin step round-trip): step 5 wipes both password fields on every validation re-render (correct for `nofilter` avoidance, wrong for UX). Extended the existing page-tail vanilla-JS guard (already covered the mismatched-password case) with SteamID format + email shape checks so the round-trip-with-wiped-passwords path stops being the common case. Server-side validation stays as the second line of defense. m1 (recovery.php direct hit): pre-fix the surface always emitted 503, even when `vendor/autoload.php` was actually present (someone bookmarked the URL, an operator visited it directly out of curiosity). Self-checks vendor presence at the top and 302s to `/install/` if present. m2 (license/licence consistency): standardized on American "License" across step 1 (page handler + template) — matches LICENSE.md, the testid prefix `install-license-*`, and the rest of the repo's spelling. m3 (testid prefix sweep): step 2 used mixed `install-db-*` (fields) and `install-database-*` (form / buttons). Standardized field testids on `install-database-*` to match every other step's pattern. m4 (PDO error translation): step 2 surfaced raw PDOException strings ("SQLSTATE[HY000] [1045] Access denied for user 'sourcebans'@'192.168.96.5' (using password: YES)") — gibberish to non-DBAs plus the panel-as-seen- by-DB internal IP is a minor information disclosure. New `sbpp_install_translate_pdo_error()` helper in `web/install/includes/helpers.php` pattern-matches the four codes a non-technical operator is most likely to hit (1045 access denied / 2002 host unreachable / 1049 unknown database / 1044 denied for user on database) and emits a friendly translation; falls back to the raw message for unrecognised codes so debugging stays possible. Regression test in `web/tests/integration/InstallGuardTest.php` (added with C1). m5 (license textarea height): step 1's `<textarea ... rows="20">` was overridden by `.input { height }` from the wizard's CSS, collapsing to one row. Switched to the panel's `.textarea` class with an inline `min-height: 24rem` so the licence is readable without scroll-noise. m6 (AMXBans step helper text): step 6's labels-only fields gave operators no hint where to find the values. Added a top-of-form "Look in `addons/amxmodx/configs/sql.cfg`" pointer plus per-field helper text mirroring `page_database.tpl`'s shape. * docs: AGENTS.md + ARCHITECTURE.md + README.md updates for sbpp#1335 Doc-and-code drift is a defect per AGENTS.md's "Keep the docs in sync" rule. Update both panels + the user-facing README to reflect the sbpp#1335 fixes: AGENTS.md: - Install wizard lifecycle: insert the C2 already-installed gate between paths-bootstrap and recovery short-circuit; document the sister-guards on either side of the wizard (panel-runtime `init-recovery.php` + wizard-side `already-installed.php`) and the new `SBPP_DEV_KEEP_INSTALL` dev-only opt-in - "Where to find what": three new rows for the friendly-error surface, the wizard's already-installed gate, and the PDO error translator - "Edit a step of the install wizard" row: refreshed to mention the new helper functions, the licence→license sweep (m2), the testid standardization (m3), the page-tail JS validation additions on step 5 (M3), and the textarea height fix (m5). - Anti-patterns: four new entries — `HTTP_HOST` magic on the guard, allowing the wizard to start over an installed panel, bare-text `die()` in `init.php`, and surfacing raw `PDOException` strings to operator-facing banners. - Wizard-vanilla-JS anti-pattern updated to mention the M3 step-5 admin-form validation extensions and standardize on "license" spelling. ARCHITECTURE.md: - Web panel directory layout: surface `init-recovery.php` and `already-installed.php` and their sbpp#1335 IDs in the tree. - Bootstrap step 2: replace the localhost-host-exemption sentence with the C1 + M1 + dev-escape-hatch description. - Local dev stack: rewrite the `dev-prepend.php` paragraph (used to be `HTTP_HOST` rewrite, now `SBPP_DEV_KEEP_INSTALL` define). - Legacy patterns table: three new rows for the C1, M1, and C2 pre-fix shapes. README.md (m7): - New paragraph between install steps 3 and 4 telling operators how to set folder permissions if the wizard's environment check complains. Matches the M2 in-product remediation hint. * Fix sbpp#1335 M2 review: split missing-folder hint from not-writable hint PR sbpp#1336's first cut of M2 appended the same chmod-flavored hint ("set permissions to 0775 ... via your hosting File Manager, FTP client, or chmod") to BOTH the `Missing:` AND `Not writable:` branches in `web/install/pages/page.3.php`. For a missing directory the operator can't chmod something that doesn't exist — they need to re-upload from the release zip (or `mkdir`). The release tarball ships a placeholder for every required folder (`web/demos/.gitkeep`, `web/cache/`, the bundled `web/images/games/*.png` and `web/images/maps/*` files), so a `Missing:` status indicates a partial / broken upload, not a permission problem. Lift the detail-string construction into `sbpp_install_describe_filesystem_check()` in `web/install/includes/helpers.php` — pure function over `(path, exists, writable)` returning the `{$row.detail}` text the template renders. Three branches, three distinct remediations: - Missing → "re-upload this folder from the release zip, or create it via your hosting File Manager." - Not writable → the existing chmod 0775 hint (paired with README m7's signpost so the two surfaces stay in sync). - OK → bare 'Writable' (no hint needed; the row already shows a green check). The pure-function shape is unit-testable. New regression test `testFilesystemCheckEmitsDistinctRemediations` in `web/tests/integration/InstallGuardTest.php` pins the contract in both directions (the missing branch must NOT mention chmod; the not-writable branch must NOT suggest re-uploading) so a future drift can't silently re-merge the two hints. * Fix sbpp#1335 M3 review: drop novalidate, native attrs gate every input case PR sbpp#1336's first cut of M3 added SteamID + email + password-match checks to the admin-form's page-tail JS — but the form carried `novalidate`, which switched off the browser's pre-submit checks for `required` / `minlength="8"` / `pattern` / `type="email"`. Two follow-up gaps: - **Short password** (`<8` chars): native `minlength` was off, JS only checked match — a 5-char password matching its 5-char confirm passed the JS guard, then bounced server-side ("Password must be at least 8 characters.") wiping both fields. - **Empty fields**: native `required` was off, JS didn't explicitly check emptiness — every empty username / email / SteamID hit the server's `'All fields are required.'` and wiped both passwords on the re-render. Both gaps are symptomatic of the same root cause: `novalidate` violates AGENTS.md's install-wizard rule that "the form's native `required` / `pattern` attributes must be the load-bearing gate, with JS as the UX polish". The fix drops `novalidate` and lets the native attrs cover empty / short / pattern / type cases the way the rule intends — the browser surfaces its popover before our submit handler runs. The JS handler shrinks to just the cross-field password-match check (the one validation native HTML can't express). On submit, native runs first; if everything natively-valid, our handler runs and `setCustomValidity('Passwords do not match.')` + `reportValidity()` + `e.preventDefault()` re-uses the same native popover surface for the custom message. `autocomplete="new-password"` + the never-echo-back-into-template contract on the password fields are unchanged; this is purely about which validations gate submission and how the operator sees the failure (native popover anchored to the field vs server-side bounce that wipes passwords). * docs: AGENTS.md install-wizard cross-field-validation contract Two paired updates riding the M3 review fix in the previous commit: 1. The "Conventions for new wizard work" block's "Forms POST natively" rule grows a `novalidate` carve-out spelling out the canonical "cross-field validation" shape. The existing rule was correct ("the form's native `required` / `pattern` attributes must be the load-bearing gate, with JS as the UX polish"), but didn't explicitly call out `novalidate` as the anti-pattern that defeats it. Future wizard steps with a genuine cross-field need (cf. step 5's password-match) follow the canonical shape: keep native validation on, hook submit, set customValidity in the handler, clear it on input. 2. The wizard-vanilla-JS anti-pattern entry (in "Anti-patterns", under the install-wizard heading) updates from "the admin form's password-match + SteamID + email shape checks" to "the admin form's cross-field password-match check" — the SteamID + email checks that PR sbpp#1336 first added are now handled natively by the form's `pattern` and `type="email"` attrs, so listing them as JS-territory was already stale. 3. The "Edit a step of the install wizard" row in "Where to find what" gets `sbpp_install_describe_filesystem_check` added to the helper list (paired with the M2 review fix's lift of the filesystem-check string-building into a pure helper).
1 parent f0571f8 commit e0dfd0e

20 files changed

Lines changed: 1511 additions & 103 deletions

AGENTS.md

Lines changed: 146 additions & 21 deletions
Large diffs are not rendered by default.

ARCHITECTURE.md

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -114,13 +114,16 @@ web/
114114
├── configs/permissions/ web.json + sourcemod.json — bitmask flag definitions
115115
├── tests/ PHPUnit (api/ for handlers, integration/ for flows)
116116
├── bin/ CLI tools (currently just generate-api-contract.php)
117+
├── init-recovery.php Panel-runtime guard helpers + friendly error pages (#1335 C1 + M1)
117118
├── install/ Install wizard self-hosters run on a fresh setup (#1332)
118-
│ ├── index.php Entry point — paths-init → vendor/-check (recovery short-circuit) → bootstrap → dispatch
119+
│ ├── index.php Entry point — paths-init → already-installed gate (#1335 C2) → vendor/-check (recovery short-circuit) → bootstrap → dispatch
119120
│ ├── init.php Paths-only bootstrap (NEVER touches vendor/)
120121
│ ├── bootstrap.php Composer + Smarty bootstrap (loaded only when vendor/ is present)
121122
│ ├── recovery.php Self-contained "vendor/ missing" surface (pure inline HTML + CSS)
122-
│ ├── pages/page.<N>.php Per-step page handlers (1=licence, …, 6=optional AMXBans import)
123+
│ ├── already-installed.php Self-contained "panel already installed" guard (#1335 C2 — pure inline HTML + CSS)
124+
│ ├── pages/page.<N>.php Per-step page handlers (1=license, …, 6=optional AMXBans import)
123125
│ ├── includes/routing.php Step → page-handler dispatch
126+
│ ├── includes/helpers.php Shared step-handler helpers (prefix validation, raw-PDO probe, KV escape, PDO error translation)
124127
│ └── includes/sql/ struc.sql + data.sql — the schema source of truth
125128
├── updater/ Per-version migrations existing installs run after upgrade
126129
├── phpstan.neon PHPStan level 5 + custom rules + dba bootstrap
@@ -147,8 +150,15 @@ Both scripts include `init.php` first, which performs identical bootstrap.
147150

148151
1. Defines path constants (`ROOT`, `INCLUDES_PATH`, `TEMPLATES_PATH`, …)
149152
and the `IN_SB` sentinel that page files check.
150-
2. Bails if `config.php` is missing or if the `install/` or `updater/`
151-
directories are present and the host isn't `localhost`.
153+
2. Redirects to `/install/` if `config.php` is missing (the
154+
wizard is the canonical fresh-install path; #1335 M1 replaced
155+
the bare-text `die()`). If `install/` or `updater/` are still
156+
on disk after a successful install/upgrade, refuses to boot via
157+
`web/init-recovery.php`'s `sbpp_check_install_guard()`
158+
unconditional in production, with a single explicit
159+
`SBPP_DEV_KEEP_INSTALL` opt-in for the docker dev stack
160+
(#1335 C1; the loopback / `HTTP_HOST` exemption was a
161+
panel-takeover path and is gone).
152162
3. Loads Composer autoload (`includes/vendor/autoload.php`).
153163
4. Manually requires the auth + security + Database modules and
154164
initialises them. The classes themselves ARE PSR-4 namespaced
@@ -913,17 +923,23 @@ model:
913923
- **`docker/php/web-entrypoint.sh`** waits for MariaDB, renders
914924
`web/config.php` from env vars (only if absent), runs `composer install`
915925
if `vendor/` is empty, then `exec`s Apache.
916-
- **`docker/php/dev-prepend.php`** rewrites `HTTP_HOST` to `localhost`
917-
for any loopback request so `init.php`'s install-folder guard accepts
918-
the forwarded `:8080` port.
926+
- **`docker/php/dev-prepend.php`** defines `SBPP_DEV_KEEP_INSTALL`
927+
on every request so `web/init-recovery.php`'s
928+
`sbpp_check_install_guard()` skips the install/ + updater/-presence
929+
refusal — the dev stack ships those directories on the bind mount
930+
by design (the wizard isn't exercised locally; `docker/db-init/`
931+
seeds the schema out of band). Pre-#1335 this file rewrote
932+
`HTTP_HOST` to `localhost` to ride a `init.php` exemption that
933+
was a panel-takeover path in production; the explicit
934+
loud-named-define escape hatch replaced it.
919935
- **`docker/db-init/00-render-schema.sh`** runs once on first DB boot:
920936
substitutes `{prefix}` / `{charset}` in `struc.sql` + `data.sql`,
921937
loads them, and seeds an `admin` row with bcrypt of `admin`.
922938
- **`sbpp.sh`** is a thin wrapper around `docker compose` plus the
923939
quality gates and DB tasks. Run `./sbpp.sh -h` for the full menu.
924940

925-
The seeded admin password and the `HTTP_HOST` shim are dev-only and
926-
documented as such in `docker-compose.yml`.
941+
The seeded admin password and the `SBPP_DEV_KEEP_INSTALL` constant are
942+
dev-only and documented as such in `docker-compose.yml`.
927943

928944
## Quality gates
929945

@@ -1073,3 +1089,6 @@ but don't bulk-rewrite legacy code without justification.
10731089
| `htmlspecialchars_decode` on JSON params | Store raw UTF-8; Smarty auto-escape handles display (#1108) |
10741090
| `DB_CHARSET = 'utf8'` (3-byte alias) | `utf8mb4` end-to-end (panel PDO + plugin `SET NAMES`) (#1108)|
10751091
| TinyMCE WYSIWYG for `dash.intro.text` | Plain `<textarea>` + `Sbpp\Markup\IntroRenderer` (CommonMark, escape unsafe HTML) (#1113) |
1092+
| `init.php`'s `HTTP_HOST != 'localhost'` exemption on the install/ + updater/-presence guard | Unconditional guard via `web/init-recovery.php`'s `sbpp_check_install_guard()`; docker dev rides explicit `SBPP_DEV_KEEP_INSTALL` constant (#1335 C1) |
1093+
| Bare-text `die()` in `init.php` for missing-config / install-still-present / autoload-missing paths | Self-contained chrome via `web/init-recovery.php`'s `sbpp_render_install_blocked_page()` (mirror of `recovery.php`'s pure-inline-HTML contract) (#1335 M1) |
1094+
| `/install/` walkable on a panel where `config.php` already exists | Wizard refuses to start via `web/install/already-installed.php`'s 409 surface (#1335 C2) |

README.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,13 @@ MariaDB, including shared hosting (cPanel, DirectAdmin, Plesk).
5454
to your web root (`public_html/`, `htdocs/`, `www/`, …) via your
5555
host's File Manager or any FTP/SFTP client. The plugin tarball
5656
goes onto your game server under `addons/sourcemod/`.
57+
58+
If the wizard's environment check (next step) flags any folder
59+
as "Not writable", set permissions to `0775` (or `0777` on shared
60+
hosts where you don't control the PHP user) on `web/demos/`,
61+
`web/cache/`, `web/images/games/`, and `web/images/maps/`. Most
62+
hosts let you do this through File Manager → Properties; over
63+
SSH it's `chmod -R 0775 web/{demos,cache,images}`.
5764
4. **Run the installer.** Visit `https://your-panel-url/install/` in
5865
a browser and follow the wizard — license, database details,
5966
environment check, schema install, admin account, done.

docker-compose.yml

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,10 @@
44
# ./sbpp.sh logs # tail web/db logs
55
# open http://localhost:8080 # panel — login admin / admin
66
#
7-
# This file is for development only. Do NOT use it in production: it disables
8-
# the install-folder safety check, ships a known admin password, and exposes
9-
# the database port to the host.
7+
# This file is for development only. Do NOT use it in production: it ships a
8+
# known admin password, exposes the database port to the host, and the dev
9+
# image defines `SBPP_DEV_KEEP_INSTALL` (issue #1335 C1) so init.php's
10+
# install/ + updater/ presence guard skips during dev.
1011

1112
services:
1213
web:

docker/Dockerfile

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,9 @@ COPY --from=composer:2 /usr/bin/composer /usr/bin/composer
4141

4242
# Sensible PHP defaults for development: errors on, generous limits, opcache
4343
# revalidates on every request so file edits are picked up immediately.
44-
# auto_prepend_file normalizes HTTP_HOST so init.php's "delete install/"
45-
# check (which only matches a bare "localhost") doesn't trip when serving on
46-
# localhost:8080.
44+
# auto_prepend_file defines `SBPP_DEV_KEEP_INSTALL` (issue #1335 C1) so
45+
# init.php's install/ + updater/ presence guard skips during dev — the
46+
# bind-mounted worktree carries both directories from git.
4747
COPY docker/php/dev-prepend.php /usr/local/etc/php/dev-prepend.php
4848
RUN { \
4949
echo 'memory_limit=256M'; \

docker/php/dev-prepend.php

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,30 @@
11
<?php
22
// Auto-prepended on every request inside the dev container.
33
//
4-
// init.php has a guard:
5-
// if ($_SERVER['HTTP_HOST'] != "localhost" && !defined("IS_UPDATE")) {
6-
// if (file_exists(ROOT."/install")) { die('Please delete the install directory'); }
7-
// }
8-
// That bare-string match doesn't accept "localhost:8080" or "127.0.0.1", so
9-
// the panel refuses to load. Strip the port for any loopback host so the
10-
// guard sees the value it expects without weakening it for real deployments.
11-
if (isset($_SERVER['HTTP_HOST'])
12-
&& preg_match('/^(localhost|127\.0\.0\.1|\[::1\])(:\d+)?$/i', $_SERVER['HTTP_HOST'])) {
13-
$_SERVER['HTTP_HOST'] = 'localhost';
4+
// Issue #1335 C1: pre-#1335 init.php exempted `HTTP_HOST == "localhost"`
5+
// from the install/ + updater/ presence guard, and this file's job was
6+
// to rewrite `HTTP_HOST` to drop the port (`localhost:8080` -> `localhost`)
7+
// so the bare-string match would accept dev requests. That entire
8+
// shape was a panel-takeover path — anyone reaching a production
9+
// panel with a `localhost` Host header (port-forward, SSH tunnel,
10+
// ngrok, Cloudflare Tunnel) bypassed the guard.
11+
//
12+
// The post-#1335 contract: init.php's guard is unconditional, with a
13+
// single explicit dev-only escape hatch. Defining `SBPP_DEV_KEEP_INSTALL`
14+
// here tells `sbpp_check_install_guard()` to skip the install/ +
15+
// updater/ presence check. The constant is loud-named so a
16+
// production-side define is visibly wrong; the panel's release
17+
// tarball has no path to set it; only the dev container's
18+
// `auto_prepend_file` ini directive (configured in
19+
// `docker/Dockerfile`) actually defines it.
20+
//
21+
// The dev container needs the escape hatch because the worktree is
22+
// bind-mounted into the panel's web root and includes both
23+
// `install/` and `updater/` from git. Deleting either is not an
24+
// option in dev — the docker-compose dev stack seeds the DB out of
25+
// band (the wizard isn't exercised), but `install/` itself stays in
26+
// place so wizard development happens against the same files that
27+
// ship to production.
28+
if (!defined('SBPP_DEV_KEEP_INSTALL')) {
29+
define('SBPP_DEV_KEEP_INSTALL', true);
1430
}

0 commit comments

Comments
 (0)