Fix #1335: post-install-wizard audit (panel-takeover paths + 8 UX gaps)#1336
Merged
Conversation
…rome 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 (#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-#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
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 #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.
Wizard-side UX fixes from the #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.
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 #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 #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.
PR #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.
PR #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).
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 #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).
Member
Author
|
Adversarial review findings addressed in three follow-up commits on this branch (be6d7b7, a194398, 8d5e635). All four quality gates re-verified green; one new regression test method added. Fixes
Quality gates re-run
P3 itemsThe reviewer's five P3 items are out-of-scope for this PR but tracked as a single follow-up at #1338 so they don't get lost — short summaries of each, ready for the maintainer to pick up later. Smoke tests
|
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Closes #1335.
End-to-end fix for the post-#1332 install-wizard human-flow audit. Two
critical security findings (panel-takeover paths) + three major UX
findings + seven minor papercuts, plus the paired regression tests
and doc updates.
Critical (security)
HTTP_HOST != "localhost"exemption oninit.php'sinstall/+updater/-presence guard. The exemption was a panel-takeover
path on any panel reachable via a
localhostHost header(port-forward, SSH tunnel, ngrok, Cloudflare Tunnel). Replaced with
an explicit
SBPP_DEV_KEEP_INSTALLconstant (loud-named, onlydefined automatically by
docker/php/dev-prepend.phpfor the devstack).
IS_UPDATEexemption preserved.Pre-fix, anyone reaching
/install/after the wizard finished couldwalk it again and overwrite
config.php/ admin / DB. The newweb/install/already-installed.phpsurface (pure inline HTML + CSS,same shape as
recovery.phpsince it runs upstream of Composer)emits 409 Conflict and explains the intentional-reinstall path
(delete
config.phpfirst).Major (UX friction at the worst possible moment)
die('SourceBans++ is not installed')/die('Please delete the install directory')/die('Composer autoload not found')calls with self-contained chromein
web/init-recovery.php'ssbpp_render_install_blocked_page().Bonus: missing-
config.phpnow redirects to/install/instead ofdying.
remediation hint.
SteamID format + email shape on submit (alongside the existing
password-match check) so the round-trip-with-wiped-passwords path
stops being the common case. Server-side validation stays as the
second line of defense.
Minor (papercuts)
recovery.php302s to/install/when vendor IS presenton a direct hit (was always 503).
install-database-*(was a mix of
install-db-*for fields,install-database-*forform/buttons).
sbpp_install_translate_pdo_error()translates the fourcommon connect-error codes (1045 / 2002 / 1049 / 1044) into
operator-friendly messages; raw message preserved as fallback for
unrecognised codes.
.input(whichoverrides height) to
.textareawithmin-height: 24remso thelicense is readable without scroll-noise.
addons/amxmodx/configs/sql.cfg+ per-field helper text mirroringstep 2's shape.
remediation between steps 3 and 4.
Tests
web/tests/integration/InstallGuardTest.php— pure-function unittests pinning:
localhostHost header does NOT bypass the install guard (C1regression)
IS_UPDATEskips the guard (preserved legacy contract)SBPP_DEV_KEEP_INSTALLskips the guard (new dev escape hatch)config.phppresencesbpp_install_translate_pdo_error()covers all four common codesand falls back gracefully on unknowns
Doc updates
Where to find what(three new rows + refresh of the existing wizard-step row), four
new Anti-patterns entries, and refresh of the wizard-vanilla-JS
anti-pattern (license spelling + M3 step-5 additions).
files), bootstrap step 2 (rewrite for the C1 + M1 contract),
local dev-stack note (
dev-prepend.phprewrite), legacy-patternstable (three new rows).
Test plan
deprecation, untouched by this PR)
/install/→ 409Already-installed ✓, deleting
config.phplets the wizard run ✓,web/init.phprecovery surfaces render under all three scenarios ✓.Deviations from the issue
None. The issue suggested either dropping the localhost exception
unconditionally OR moving it behind a
define('SBPP_DEV_KEEP_INSTALL', true)opt-in; this PR takes the second path so the docker devworkflow keeps working without operator intervention. Rationale:
the constant is loud-named (visibly wrong if defined in production),
defined only by
docker/php/dev-prepend.php'sauto_prepend_file(production panels have no path to define it), and documented as a
dev-only escape hatch in
AGENTS.mdConventions + Anti-patterns.