From 1fa7c8f2c925cf49a60a38c635e4d39af41a6c13 Mon Sep 17 00:00:00 2001 From: rumblefrog Date: Sun, 10 May 2026 13:59:44 -0400 Subject: [PATCH] Fix #1314: rename reused :sid placeholder in admin.srvadmins.php MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin-list SELECT in `pages/admin.srvadmins.php` mentioned the named placeholder `:sid` twice (one in the outer `WHERE server_id = :sid`, one in the inner subquery's `WHERE server_id = :sid`) but called `bind(':sid', …)` only ONCE. Under emulated prepares (PDO's pre-#1124 default), client-side substitution rewrote every `:sid` occurrence to the literal value before the SQL hit MariaDB, so the duplicate-name pattern Just Worked. After #1124 / #1167 flipped `PDO::ATTR_EMULATE_PREPARES` to `false` (so `LIMIT '0','30'` would stop tripping MariaDB strict mode), the MySQL driver started expanding each `:name` occurrence into its own positional `?` slot in the prepared statement — `bind()`-ing one occurrence leaves the others unbound and `execute()` raises `SQLSTATE[HY093] Invalid parameter number`. Result: every admin with `ADMIN_LIST_SERVERS` who clicked **Server Management → Admins** after upgrading to v2.0 hit a page-load-blocking PHP fatal with no UI workaround. The fix renames the inner placeholder to `:sid_inner` so each position is bound separately. While here, also: - Add the missing `global $userbank;` declaration. The page used `$userbank` without declaring it global; this worked in production because `core/header.php` runs first inside `build()` and imports it into the function scope, but it was a latent bug that broke the new test harness and the corresponding `Variable $userbank might not be defined` baseline entry was sitting in `phpstan-baseline.neon`. Drop the baseline entry now that the page declares the global itself, matching every other admin page. - Lift `(int) $_GET['id']` into a local `$sid` so the value isn't re-cast across the SELECT bind + the `checkMultiplePlayers` call, and coalesce on `null` so the read survives PHP 9's stricter null-into-scalar rules. Regression coverage in `web/tests/integration/SrvAdminsPdoParamTest.php` (two methods): - `testReusedNamedPlaceholderUnderNativePreparesIsRejected` — issues a tiny `SELECT 1 ... WHERE aid = :sid OR aid = :sid` against `Sbpp\Db\Database` with one `bind()` and asserts it throws `HY093`. Pins the contract; also a regression guard if anyone re-flips `EMULATE_PREPARES` back to `true`. - `testAdminSrvadminsPageRendersWithoutPdoException` — process- isolated `require` of the page handler with `?id=0` asserts no `PDOException` escapes. Pre-fix this test fails at `Database.php:101` with the exact stack from the issue trace. Docs updated in the same PR per AGENTS.md "Keep the docs in sync": - `AGENTS.md` adds the `:name`-binding rule under the "Database" convention, the matching "Anti-patterns" entry, and a "Where to find what" row pointing at the new test. - `ARCHITECTURE.md` documents the two practical consequences of `EMULATE_PREPARES => false` for callers (binary-protocol numerics AND each `:name` occurrence expanding to its own slot) under the Database subsystem walkthrough. --- AGENTS.md | 39 ++++ ARCHITECTURE.md | 19 ++ web/pages/admin.srvadmins.php | 23 ++- web/phpstan-baseline.neon | 6 - .../integration/SrvAdminsPdoParamTest.php | 175 ++++++++++++++++++ 5 files changed, 252 insertions(+), 10 deletions(-) create mode 100644 web/tests/integration/SrvAdminsPdoParamTest.php diff --git a/AGENTS.md b/AGENTS.md index 07ac8f017..9e79fd5b4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -357,6 +357,26 @@ top-level `class Foo {}` in `web/includes/` (see "Anti-patterns"). `Database::query()` rewrites the placeholder. Never inline the prefix. - Pattern: `query` → `bind` → `execute` / `single` / `resultset`. - ADOdb was fully removed (commit `b9c812b2`). **Do not reintroduce it.** +- Each named placeholder (`:name`) inside one query needs as many + `bind()` calls as occurrences. The panel runs PDO with + `PDO::ATTR_EMULATE_PREPARES => false` (`Sbpp\Db\Database::__construct` + — set at #1124 / motivated by #1167 so `LIMIT '0','30'` stops + tripping MariaDB strict mode). Under native prepares the MySQL + driver expands every `:name` occurrence into its own positional + `?` slot in the prepared statement, so reusing `:sid` twice and + `bind(':sid', …)` once leaves the second slot unbound and + `execute()` raises `SQLSTATE[HY093] Invalid parameter number` + (#1314). Pre-#1124 emulated prepares masked this by client-side + string substitution at every occurrence. Either rename each + occurrence (`:sid` + `:sid_inner`) and `bind()` each, or pass the + values via the `resultset(['sid' => …, 'sid_inner' => …])` array + shortcut — both shapes are equivalent. The `:prefix_` literal is + not a real PDO placeholder; it's a substring that + `Database::setPrefix()` replaces before `prepare()`, so reuse + there is harmless. Regression guard: + `web/tests/integration/SrvAdminsPdoParamTest.php` pins both the + contract (single-bind on a reused name throws `HY093`) and the + page-level fix (`admin.srvadmins.php` renders without raising). ### PHP 8.5 idioms (post-#1289 floor bump) @@ -1097,6 +1117,24 @@ audit (#1207) locked in. New CTAs: - `utf8` (3-byte alias) for `DB_CHARSET` → always `utf8mb4`. 4-byte sequences (emoji, some CJK) otherwise trip `Incorrect string value` from the plugin's insert path (#1108, #765). +- Reusing the same `:name` PDO placeholder more than once inside a + query while calling `bind(':name', …)` only ONCE → the panel runs + PDO with `EMULATE_PREPARES => false` (default since #1124 / #1167's + `LIMIT '0','30'` MariaDB regression), so the MySQL driver expands + every `:name` occurrence into its own positional `?` slot in the + prepared statement. A single `bind()` leaves the others unbound + and `execute()` raises `SQLSTATE[HY093] Invalid parameter number` + (#1314 — `admin.srvadmins.php`'s `:sid` / `:sid` / `bind(':sid', …)` + shape, which Just Worked under emulated prepares pre-#1124 and + fataled on every page load post-#1124). Either rename each + occurrence (`:sid` + `:sid_inner`) and `bind()` each, or pass the + values via `resultset(['sid' => …, 'sid_inner' => …])`. The + `:prefix_` literal is rewritten by `Database::setPrefix()` before + `prepare()`, so reuse there is harmless and stays out of this + rule. Re-flipping `EMULATE_PREPARES` back to `true` to mask the + bug is a sibling anti-pattern — it would silently reintroduce the + `LIMIT '0','30'` trap (`page.banlist.php` / `page.commslist.php` + rejected by MariaDB strict mode). See "Database" under Conventions. - Editing `install/includes/sql/data.sql` (or `struc.sql`) without a paired `web/updater/data/.php` → upgraded installs silently miss the change. - WYSIWYG / "rich HTML" editors (TinyMCE, CKEditor, …) for fields stored @@ -1287,6 +1325,7 @@ audit (#1207) locked in. New CTAs: | API wire-format snapshots | `web/tests/api/__snapshots__//.json` | | Action -> permission lock | `web/tests/api/PermissionMatrixTest.php` | | Trap PHP 8.1 null-into-scalar deprecations at runtime (the bits PHPStan can't see) | `web/tests/integration/Php82DeprecationsTest.php` (#1273) — process-isolated render harness with a stub Smarty + `set_error_handler` that promotes `E_DEPRECATED` / `E_USER_DEPRECATED` to `\ErrorException`. Mirrors the LostPasswordChromeTest stub-Smarty pattern; each test method runs in a separate process because the page handlers declare top-level helpers (`setPostKey()` etc.) that PHP can't redeclare in one process. Add a marquee route here whenever a new high-traffic page handler ships, especially if it reads nullable `:prefix_*` columns or `$_POST` / `$_GET` lookups. | +| Pin the "every `:name` PDO placeholder needs as many `bind()` calls as occurrences" contract under native prepares | `web/tests/integration/SrvAdminsPdoParamTest.php` (#1314) — two methods. `testReusedNamedPlaceholderUnderNativePreparesIsRejected` issues a tiny `SELECT 1 ... WHERE aid = :sid OR aid = :sid` against `Sbpp\Db\Database` with one `bind()` and asserts it throws `HY093`; this is the contract pin (also a regression guard if anyone re-flips `EMULATE_PREPARES` back to `true`). `testAdminSrvadminsPageRendersWithoutPdoException` is the page-level regression guard for the actual #1314 fatal — process-isolated `require` of `pages/admin.srvadmins.php` with `?id=0` asserting no `PDOException` escapes. Mirrors the Php82DeprecationsTest stub-Smarty + process-isolation shape. | | Add an E2E spec | `web/tests/e2e/specs//...` + `web/tests/e2e/pages/...` | | Add a route to the screenshot gallery | `web/tests/e2e/specs/_screenshots.spec.ts` (`ROUTES` array) | | Tweak mobile (<=768px) chrome layout | `web/themes/default/css/theme.css` — see the `#1207` `@media (max-width: 768px)` blocks for the canonical shapes (icon-only topbar search, full-width drawer + scroll lock). Sub-paged admin routes (servers / mods / groups / comms / settings / admins / bans) use the `
` accordion in the `#1259` `@media (min-width: 1024px)` block (sidebar inline at `<1024px`, sticky 14rem rail at `>=1024px`); see "Sub-paged admin routes" in Conventions. | diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index d4c59c87c..bc12aff36 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -341,6 +341,25 @@ A thin PDO wrapper. Two things to know: $row = $GLOBALS['PDO']->single(); // or ->resultset() / ->execute() ``` +- The constructor sets `PDO::ATTR_EMULATE_PREPARES => false` (added at + #1124 / motivated by #1167's `LIMIT '0','30'` MariaDB strict-mode + regression). Two practical consequences for callers: numeric values + go through MySQL's binary protocol with proper type metadata + (`LIMIT ?,?` works as expected), AND every named placeholder + occurrence is expanded into its own positional `?` slot in the + prepared statement — so a query that mentions `:sid` twice needs + TWO `bind(':sid', …)` calls (or distinct names like `:sid` + + `:sid_inner`), otherwise `execute()` raises `SQLSTATE[HY093] + Invalid parameter number`. Pre-#1124 emulated prepares masked + the duplicate-name pattern by client-side string substitution at + every occurrence. The contract is pinned by + `web/tests/integration/SrvAdminsPdoParamTest.php` (the regression + guard for #1314, where `pages/admin.srvadmins.php` reused `:sid` + twice and bound once — page-load-blocking fatal for every admin + with `ADMIN_LIST_SERVERS` after upgrading to v2.0). The `:prefix_` + placeholder is rewritten by `setPrefix()` BEFORE `prepare()`, so + `:prefix_admins` reuse is harmless and stays out of this rule. + The legacy ADOdb layer was fully removed in commit `b9c812b2`; do not reintroduce it. PHPStan + `staabm/phpstan-dba` introspect the live schema (rendered from `install/includes/sql/struc.sql`) and type-check diff --git a/web/pages/admin.srvadmins.php b/web/pages/admin.srvadmins.php index eeec37582..5bc35cd4d 100644 --- a/web/pages/admin.srvadmins.php +++ b/web/pages/admin.srvadmins.php @@ -17,13 +17,27 @@ Page: - *************************************************************************/ -global $theme; +global $userbank, $theme; new AdminTabs([], $userbank, $theme); $admsteam = []; $admins = []; +// Issue #1314: under native prepares (`PDO::ATTR_EMULATE_PREPARES => +// false`, the panel's default since #1124 / motivated by #1167's +// `LIMIT '0','30'` MariaDB regression), the MySQL driver expands every +// `:name` occurrence into its own positional `?` slot in the prepared +// statement. A name reused inside one query therefore needs as many +// bind() calls as occurrences — `bind(':sid', …)` ONCE on a query +// that mentions `:sid` twice leaves the second slot unbound and +// `execute()` raises `SQLSTATE[HY093] Invalid parameter number`. Pre +// #1124 the duplicate-name pattern was masked by emulated prepares +// substituting the literal value client-side at every occurrence. +// The inner subquery's placeholder is renamed to `:sid_inner` so each +// position is bound separately. See AGENTS.md Anti-patterns ("Reusing +// a `:name` placeholder ..."). +$sid = (int) ($_GET['id'] ?? 0); $GLOBALS['PDO']->query("SELECT authid, user FROM `:prefix_admins_servers_groups` AS asg LEFT JOIN `:prefix_admins` AS a ON a.aid = asg.admin_id @@ -31,10 +45,11 @@ ( SELECT group_id FROM `:prefix_servers_groups` - WHERE server_id = :sid) + WHERE server_id = :sid_inner) ) GROUP BY aid, authid, srv_password, srv_group, srv_flags, user "); -$GLOBALS['PDO']->bind(':sid', (int) $_GET['id']); +$GLOBALS['PDO']->bind(':sid', $sid); +$GLOBALS['PDO']->bind(':sid_inner', $sid); $srv_admins = $GLOBALS['PDO']->resultset(); $i = 0; foreach ($srv_admins as $admin) { @@ -42,7 +57,7 @@ $admsteam[] = $admin['authid']; } } -if (count($admsteam) > 0 && $serverdata = checkMultiplePlayers((int) $_GET['id'], $admsteam)) { +if (count($admsteam) > 0 && $serverdata = checkMultiplePlayers($sid, $admsteam)) { $noproblem = true; } foreach ($srv_admins as $admin) { diff --git a/web/phpstan-baseline.neon b/web/phpstan-baseline.neon index 00b7f8b6d..d7c7da5cc 100644 --- a/web/phpstan-baseline.neon +++ b/web/phpstan-baseline.neon @@ -312,12 +312,6 @@ parameters: count: 7 path: pages/admin.settings.php - - - message: '#^Variable \$userbank might not be defined\.$#' - identifier: variable.undefined - count: 1 - path: pages/admin.srvadmins.php - - message: '#^Variable \$title might not be defined\.$#' identifier: variable.undefined diff --git a/web/tests/integration/SrvAdminsPdoParamTest.php b/web/tests/integration/SrvAdminsPdoParamTest.php new file mode 100644 index 000000000..a18648493 --- /dev/null +++ b/web/tests/integration/SrvAdminsPdoParamTest.php @@ -0,0 +1,175 @@ +assign(...)` and `$theme->display(...)`; we don't care + * about the rendered output, only that the PHP code path + * (including the SELECT under test) runs without raising. + * Mirrors `Php82DeprecationsTest::makeStubTheme()`. + */ + private function makeStubTheme(): Smarty + { + return new class extends Smarty { + /** @phpstan-ignore method.childParameterType */ + public function assign($tpl_var, $value = null, $nocache = false, $scope = null) + { + return $this; + } + + public function display($template = null, $cache_id = null, $compile_id = null) + { + return ''; + } + }; + } + + /** + * Wire the globals every page handler under `web/pages/*` reads + * straight from `$GLOBALS` (`$theme`, `$userbank`, `$PDO`). The + * production `init.php` does this on every real request; for the + * test we mirror what `Php82DeprecationsTest::bootRenderHarness()` + * sets up so the require'd page handler finds the same names. + */ + private function bootRenderHarness(): Smarty + { + $theme = $this->makeStubTheme(); + $GLOBALS['theme'] = $theme; + $GLOBALS['userbank'] = $GLOBALS['userbank'] ?? new \CUserManager(null); + $GLOBALS['username'] = $GLOBALS['username'] ?? 'tester'; + + return $theme; + } + + /** + * Standalone contract pin: under the panel's production PDO + * options (`EMULATE_PREPARES => false`, set in + * `Sbpp\Db\Database::__construct`), reusing the same `:name` + * placeholder more than once with a SINGLE `bind()` call leaves + * the second slot unbound and `PDOStatement::execute()` raises + * `SQLSTATE[HY093] Invalid parameter number`. + * + * Constructing a fresh `Database` (rather than reaching for + * `$GLOBALS['PDO']`) keeps the test self-contained — and proves + * the assertion is about `Database`'s configuration (the + * `EMULATE_PREPARES => false` option in the constructor), not + * about whatever stale state another test might have left on + * the global wrapper. + * + * The `:prefix_admins` table is one of the tables `Fixture` seeds + * by default (the admin row from `seedAdmin`), so the SELECT + * itself has something to query — the assertion is purely on + * the parameter-binding mismatch, not on row presence. + */ + public function testReusedNamedPlaceholderUnderNativePreparesIsRejected(): void + { + $db = new Database(DB_HOST, DB_PORT, DB_NAME, DB_USER, DB_PASS, DB_PREFIX, DB_CHARSET); + + // Two `:sid` occurrences, one `bind()` call. This is the + // exact failure mode `admin.srvadmins.php` exhibited before + // the fix. + $db->query('SELECT 1 FROM `:prefix_admins` WHERE aid = :sid OR aid = :sid LIMIT 1'); + $db->bind(':sid', 1); + + $this->expectException(PDOException::class); + $this->expectExceptionMessageMatches('/HY093|Invalid parameter number/i'); + + $db->resultset(); + } + + /** + * The headline regression: hitting `?p=admin&c=servers&o=admincheck&id=` + * pre-fix raised `PDOException` from the SELECT in + * `admin.srvadmins.php` (line 38 in the issue trace). The test + * requires the page file with `$_GET['id']` set so the same + * code path runs, and asserts no exception escapes. + * + * Process-isolation matches `Php82DeprecationsTest`: the page + * handler defines top-level helpers / variables PHP can't + * redeclare in a single process across multiple test cases, and + * we want a clean `$_GET` / `$_SESSION` / `$GLOBALS` per case. + * + * `$_GET['id']` is set to `0` deliberately — the seeded test DB + * has no `:prefix_admins_servers_groups` rows, so any server + * id (real or sentinel `0`) returns an empty result set. The + * regression is on the SELECT's PREPARE + EXECUTE step, which + * runs whether the result set is empty or not. Setting `0` + * also avoids needing to seed a server row just for the test. + * + * `checkMultiplePlayers($sid, ...)` is gated on + * `count($admsteam) > 0`; with an empty result set the call is + * skipped, so we don't risk a real RCON socket open against an + * arbitrary IP. + */ + #[RunInSeparateProcess] + #[PreserveGlobalState(false)] + public function testAdminSrvadminsPageRendersWithoutPdoException(): void + { + $this->loginAsAdmin(); + $this->bootRenderHarness(); + + $_SESSION = []; + $_GET = ['p' => 'admin', 'c' => 'servers', 'o' => 'admincheck', 'id' => '0']; + + ob_start(); + try { + require ROOT . 'pages/admin.srvadmins.php'; + } finally { + ob_end_clean(); + } + + $this->assertTrue(true, + 'admin.srvadmins.php must prepare + execute its admin-list SELECT ' + . 'without raising PDOException HY093 (issue #1314).'); + } +}