Fix #1314: rename reused :sid placeholder in admin.srvadmins.php

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/<N>.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__/<topic>/<scenario>.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/<smoke|flows|a11y|responsive>/...` + `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 `<details open>` 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. |

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

web/pages/admin.srvadmins.php

@@ -17,32 +17,47 @@
 Page: <http://www.sourcebans.net/> - <http://www.gameconnect.net/>
 *************************************************************************/
 
-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
     WHERE (server_id = :sid OR srv_group_id = ANY
     (
             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) {
     if ($admin['authid'] !== null) {
         $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) {

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

web/tests/integration/SrvAdminsPdoParamTest.php

@@ -0,0 +1,175 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sbpp\Tests\Integration;
+
+use PDOException;
+use PHPUnit\Framework\Attributes\PreserveGlobalState;
+use PHPUnit\Framework\Attributes\RunInSeparateProcess;
+use Sbpp\Db\Database;
+use Sbpp\Tests\ApiTestCase;
+use Smarty\Smarty;
+
+/**
+ * Issue #1314: `web/pages/admin.srvadmins.php` issued a query that
+ * 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`.
+ *
+ * The fix renames the inner placeholder to `:sid_inner` and binds
+ * both. The two test methods here pin both halves of the contract:
+ *
+ *   1. {@see testReusedNamedPlaceholderUnderNativePreparesIsRejected}
+ *      — a small standalone query against `Sbpp\Db\Database` that
+ *      reproduces the offending shape (one `bind()` for two `:sid`
+ *      slots) and asserts it throws `HY093`. This documents WHY the
+ *      rename in `admin.srvadmins.php` matters; if a future
+ *      contributor "tidies up" the SQL back into `:sid` + a single
+ *      bind, this test fails before the page does in production.
+ *      It also doubles as a regression guard if anyone re-flips
+ *      `EMULATE_PREPARES` back to `true` (which would silently
+ *      reintroduce the original masking).
+ *
+ *   2. {@see testAdminSrvadminsPageRendersWithoutPdoException} — the
+ *      end-to-end shape: requires the page handler with `$_GET['id']`
+ *      set, asserts the SELECT prepares + executes cleanly through
+ *      the global `$PDO` wrapper. Mirrors the
+ *      Php82DeprecationsTest stub-Smarty + process-isolation pattern
+ *      so the page handler's top-level helper declarations don't
+ *      collide across cases and `$_GET` / `$_SESSION` state stays
+ *      clean per case.
+ */
+final class SrvAdminsPdoParamTest extends ApiTestCase
+{
+    /**
+     * Capture-only Smarty stub. The page handler calls
+     * `$theme->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=<sid>`
+     * 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).');
+    }
+}