fix(1439): kick-only flow no longer tells the player they were banned

Right-clicking a player on `?p=servers` and choosing "Kick player" loads
the kickit iframe, which until now ran the post-ban-completion code path
unconditionally — emitting the rcon kick reason
"You have been banned by this server, check $domain for more info" and
re-attributing whatever active ban happened to share the SteamID to the
kick target's server (UPDATE :prefix_bans SET sid = ... WHERE authid = ...
AND RemovedBy IS NULL). The reporter on #1439 saw the wrong message; the
silent audit-trail corruption was the worse half of the bug.

The fix carries an explicit `mode` signal end-to-end:

- The context menu's Kick URL now appends `&mode=kick` (the post-ban
  iframe embed inside `admin.bans.php`'s "Ban Added" dialog stays on the
  default `'ban'` mode).
- `admin.kickit.php` allowlists `$_GET['mode']` to `'ban'|'kick'`
  (anything else coerces to `'ban'` — backward-compat with pre-#1439
  callers that don't supply the param).
- `KickitView` carries the mode through to `page_kickit.tpl` which
  branches the `<title>` ("Kick player" vs "Ban player"), surfaces a
  `data-mode` attribute on the container for third-party theme styling,
  and forwards the value as `mode` on every `kickit.kick_player` JSON
  call.
- `api_kickit_kick_player` re-validates the mode and gates two things on
  it: the `:prefix_bans` UPDATE is skipped on kick mode (extracted into
  `_api_kickit_should_update_ban_sid` for testability + clarity), and
  the rcon kick reason is now "You have been kicked from this server"
  on kick mode (still "You have been banned by this server, check ..."
  on ban mode).
- The iframe's post-completion redirect lands the operator back on
  `?p=servers` for the kick flow (where they came from), preserving the
  existing `?p=admin&c=bans` destination for ban mode.

Coverage:

- `KickitTest` (15 cases, 82 assertions) — handler-shape coverage for
  both modes + the unknown-mode coercion, the rcon-message branch via
  `_api_kickit_build_kick_message`, and the ban-UPDATE gate via the new
  `_api_kickit_should_update_ban_sid` helper (incl. a static-analysis
  guard that the handler invokes the helper rather than inlining the
  `$mode === 'ban'` check around the UPDATE).
- `kickit-iframe.spec.ts` adds an E2E case driving the full kick flow:
  title says "Kick player", `kickit.kick_player` payload carries
  `mode: 'kick'`, and the post-completion `window.location` flip lands
  on `index.php?p=servers` (anchored on real `waitForURL` against the
  actual 5s redirect timer — source-grep on the script body would
  silently accept a regression that deleted the kick-mode arm).
- `server-player-context-menu.spec.ts` updated to expect `&mode=kick`
  in the Kick item's href.

Documentation in AGENTS.md (context-menu prose + "Where to find what"
table + regression-guards section) extended to spell the `mode`
contract end-to-end so future readers don't re-discover the failure
mode.

Adversarial reviewer pass also caught: ban-mode UPDATE SQL was
incorrectly transcribed as `removetype = 'X'` in AGENTS.md (now
`sid = :sid WHERE authid = :authid AND RemovedBy IS NULL`); the new
unit tests now also assert the `mode` field is consumed by the handler
and not echoed back in the API response.

Fixes: #1439

Author: rumblefrog

AGENTS.md

@@ -45,6 +45,31 @@ function api_kickit_load_servers(array $params): array
  * Try to kick a single player on a single server. Used per-row by the
  * iframe's per-server JS loop.
  *
+ * The iframe is invoked from two different surfaces with subtly
+ * different semantics (#1439):
+ *
+ *  - **`mode === 'ban'`** (default, backward-compat) — the iframe is
+ *    embedded inside the "Ban Added" success dialog on
+ *    `admin.bans.php`. A ban row was JUST inserted; the per-server
+ *    loop's job is to (a) record which server actually executed the
+ *    kick on the just-created ban row (`UPDATE :prefix_bans SET sid`),
+ *    and (b) tell the player they've been banned via the rcon kick
+ *    message so they know to visit the panel for appeals.
+ *  - **`mode === 'kick'`** — the iframe loaded standalone from the
+ *    right-click context menu on the public servers page (`?p=servers`
+ *    via `web/scripts/server-context-menu.js`). There is NO ban row;
+ *    the operator just wants to kick the player without banning. We
+ *    MUST skip the `:prefix_bans` UPDATE here — without `mode`, the
+ *    UPDATE would silently re-attribute ANY of the player's existing
+ *    active bans (`RemovedBy IS NULL`) to whatever server happened to
+ *    be the kick target, corrupting the audit trail (#1439 secondary
+ *    impact). The rcon kick message is also wrong for this flow —
+ *    the player isn't banned and CAN rejoin, so "You have been
+ *    banned…" lies to them (#1439 user-reported symptom).
+ *
+ * Unrecognised `mode` values are coerced to `'ban'` (backward-compat
+ * — old callers that don't supply the param keep working).
+ *
  * @return array{
  *   status: 'kicked'|'not_found'|'no_connect',
  *   hostname: string,
@@ -60,6 +85,14 @@ function api_kickit_kick_player(array $params): array
     $sid   = (int)($params['sid']   ?? 0);
     $num   = (int)($params['num']   ?? 0);
     $type  = (int)($params['type']  ?? 0);
+    $mode  = (string)($params['mode'] ?? 'ban');
+    if ($mode !== 'kick') {
+        // Strict allowlist: only 'kick' enters the no-UPDATE / kicked-message branch.
+        // Everything else (including unsupplied + hostile + typo) falls through to
+        // 'ban' — the historical default before #1439, so old iframe embeds keep
+        // working without a paired ban-side flip.
+        $mode = 'ban';
+    }
 
     // #1423 follow-up #4 — gate the `check` shape BEFORE we reach the
     // `SteamID::compare()` call below. For `type === 0` (Steam-ID
@@ -117,28 +150,37 @@ function api_kickit_kick_player(array $params): array
         $hostname = '';
     }
 
+    $kickMessage    = _api_kickit_build_kick_message($mode, Host::complete());
+    $shouldUpdateBan = _api_kickit_should_update_ban_sid($mode);
+
     foreach (parseRconStatus($ret) as $player) {
         if ($type === 0) {
             if (SteamID::compare($player['steamid'], $check)) {
-                $GLOBALS['PDO']->query("UPDATE `:prefix_bans` SET sid = :sid WHERE authid = :authid AND RemovedBy IS NULL");
-                $GLOBALS['PDO']->bind(':sid', $sid);
-                $GLOBALS['PDO']->bind(':authid', $check);
-                $GLOBALS['PDO']->execute();
-
-                $domain = Host::complete();
-                rcon("kickid {$player['id']} \"You have been banned by this server, check $domain for more info\"", $sid);
+                if ($shouldUpdateBan) {
+                    // Track which server executed the kick on the just-created
+                    // ban row. Only meaningful when a ban exists — see the
+                    // docblock and #1439 for the kick-only data-integrity
+                    // concern that motivated gating this UPDATE on `mode`.
+                    $GLOBALS['PDO']->query("UPDATE `:prefix_bans` SET sid = :sid WHERE authid = :authid AND RemovedBy IS NULL");
+                    $GLOBALS['PDO']->bind(':sid', $sid);
+                    $GLOBALS['PDO']->bind(':authid', $check);
+                    $GLOBALS['PDO']->execute();
+                }
+
+                rcon("kickid {$player['id']} \"$kickMessage\"", $sid);
 
                 return ['status' => 'kicked', 'sid' => $sid, 'num' => $num, 'hostname' => $hostname, 'ip' => $sdata['ip'], 'port' => $sdata['port']];
             }
         } elseif ($type === 1) {
             if (($player['ip'] ?? null) === $check) {
-                $GLOBALS['PDO']->query("UPDATE `:prefix_bans` SET sid = :sid WHERE ip = :ip AND RemovedBy IS NULL");
-                $GLOBALS['PDO']->bind(':sid', $sid);
-                $GLOBALS['PDO']->bind(':ip', $check);
-                $GLOBALS['PDO']->execute();
+                if ($shouldUpdateBan) {
+                    $GLOBALS['PDO']->query("UPDATE `:prefix_bans` SET sid = :sid WHERE ip = :ip AND RemovedBy IS NULL");
+                    $GLOBALS['PDO']->bind(':sid', $sid);
+                    $GLOBALS['PDO']->bind(':ip', $check);
+                    $GLOBALS['PDO']->execute();
+                }
 
-                $domain = Host::complete();
-                rcon("kickid {$player['id']} \"You have been banned by this server, check $domain for more info\"", $sid);
+                rcon("kickid {$player['id']} \"$kickMessage\"", $sid);
 
                 return ['status' => 'kicked', 'sid' => $sid, 'num' => $num, 'hostname' => $hostname, 'ip' => $sdata['ip'], 'port' => $sdata['port']];
             }
@@ -147,3 +189,65 @@ function api_kickit_kick_player(array $params): array
 
     return ['status' => 'not_found', 'sid' => $sid, 'num' => $num, 'hostname' => $hostname, 'ip' => $sdata['ip'], 'port' => $sdata['port']];
 }
+
+/**
+ * Build the rcon `kickid` reason string for the kickit flow.
+ *
+ * Factored out of {@link api_kickit_kick_player()} so the
+ * mode-branching contract is unit-testable without standing up a real
+ * RCON probe + matching status response (the rcon `kickid` round-trip
+ * sits behind a UDP socket that the integration test surface
+ * deliberately doesn't mock — see `web/includes/system-functions.php`
+ * `rcon()`).
+ *
+ * The kick-only branch's message is intentionally short: no domain
+ * suffix, because a kicked-not-banned player has nothing to look up
+ * on the panel side and the rcon `kickid` reason field is single-line
+ * — long suffixes truncate awkwardly in the user's disconnect dialog.
+ *
+ * @param 'ban'|'kick' $mode  caller is responsible for the allowlist
+ *                            (the handler coerces unrecognised values
+ *                            to 'ban' before calling here)
+ * @param string $domain      output of {@see Host::complete()} —
+ *                            only consumed by the 'ban' branch; passed
+ *                            unconditionally so the signature is
+ *                            mode-agnostic and easier to unit-test
+ */
+function _api_kickit_build_kick_message(string $mode, string $domain): string
+{
+    return $mode === 'kick'
+        ? 'You have been kicked from this server'
+        : "You have been banned by this server, check $domain for more info";
+}
+
+/**
+ * Decide whether to run the `UPDATE :prefix_bans SET sid = :sid …` write
+ * that records which server executed the kick on the just-created ban row.
+ *
+ * Factored out of {@link api_kickit_kick_player()} for the same reason
+ * {@link _api_kickit_build_kick_message()} was: it pins the
+ * mode-branching contract in a unit-testable shape WITHOUT standing up
+ * a real RCON probe + matching status response (the
+ * `kicked`-branch UPDATE site sits BEHIND `rcon('status', $sid)`'s UDP
+ * socket, which the integration test surface deliberately doesn't
+ * mock — see `system-functions.php` `rcon()`). Without this extraction
+ * the only way to verify "kick mode does NOT mutate ban rows" would be
+ * to stub `rcon()` itself, which is a separate refactor. The grep-shaped
+ * static regression guard
+ * (`KickitTest::testHandlerInvokesShouldUpdateHelperBeforeUpdate`)
+ * confirms the handler still calls this helper at the right place;
+ * the unit test confirms the helper returns the right verdict per mode.
+ *
+ * @param 'ban'|'kick' $mode  caller is responsible for the allowlist
+ *                            (the handler coerces unrecognised values
+ *                            to 'ban' before calling here)
+ * @return bool  `true` when the handler should run the UPDATE, `false`
+ *               when it should skip it (kick-only flow has no ban row
+ *               to attribute the kick to, and running the UPDATE would
+ *               silently re-attribute ANY of the kicked player's
+ *               existing active bans to the kick-target server)
+ */
+function _api_kickit_should_update_ban_sid(string $mode): bool
+{
+    return $mode === 'ban';
+}

web/api/handlers/kickit.php

@@ -19,6 +19,13 @@
  *   - `$check` / `$type`: pass-through query params forwarded into
  *     {@link api_kickit_kick_player()} (steam id / ip + integer
  *     discriminator).
+ *   - `$mode`: `'ban'` (post-ban-kick flow embedded in the
+ *     "Ban Added" iframe on `admin.bans.php`, default) or `'kick'`
+ *     (standalone kick-only flow from the right-click context menu on
+ *     `?p=servers`). Drives both the rendered page heading copy AND
+ *     the `mode` param forwarded to
+ *     {@link api_kickit_kick_player()}. See `web/pages/admin.kickit.php`
+ *     for the URL-param allowlist (#1439).
  *   - `$servers`: per-row markers for the polling JS; the
  *     {@link api_kickit_load_servers()} JSON action refreshes the
  *     rcon-availability flag at runtime.
@@ -37,13 +44,21 @@ final class KickitView extends View
 
     /**
      * @param list<array{num:int, ip:string, port:string|int}> $servers
+     * @param 'ban'|'kick' $mode
+     *
+     * `$mode` carries an inline default of `'ban'` so call sites that
+     * predate #1439 keep working without an audit (the post-ban iframe
+     * embed in `admin.bans.php` is the canonical "no mode supplied"
+     * caller). New callers should pass `'kick'` explicitly when they
+     * mean the standalone kick flow.
      */
     public function __construct(
         public readonly string $csrf_token,
         public readonly int $total,
         public readonly string $check,
         public readonly int $type,
         public readonly array $servers,
+        public readonly string $mode = 'ban',
     ) {
     }
 }

web/includes/View/KickitView.php

@@ -28,13 +28,26 @@
     $num++;
 }
 
+// #1439 — allowlist the `mode` URL param so the iframe can tell the
+// post-ban-kick flow (admin.bans.php "Ban Added" success dialog,
+// default) apart from the kick-only flow (right-click context menu on
+// the public servers page, `&mode=kick`). The handler branches on
+// this string to (a) skip the `:prefix_bans` UPDATE that's only
+// meaningful when a ban row exists, and (b) emit the matching rcon
+// kick message ("kicked" vs "banned"). Anything other than 'kick'
+// falls through to 'ban' (backward-compat — pre-#1439 callers don't
+// supply the param).
+$rawMode = (string) ($_GET['mode'] ?? 'ban');
+$mode    = $rawMode === 'kick' ? 'kick' : 'ban';
+
 $theme->setLeftDelimiter('-{');
 $theme->setRightDelimiter('}-');
 \Sbpp\View\Renderer::render($theme, new \Sbpp\View\KickitView(
     csrf_token: CSRF::token(),
     total: count($serverlinks),
     check: (string) ($_GET['check'] ?? ''),
     type: (int) ($_GET['type'] ?? 0),
+    mode: $mode,
     servers: $serverlinks,
 ));
 $theme->setLeftDelimiter('{');

web/pages/admin.kickit.php

@@ -351,7 +351,25 @@
  */
 /**
  * Try to kick a single player on a single server. Used per-row by the iframe's
- * per-server JS loop.
+ * per-server JS loop.  The iframe is invoked from two different surfaces with
+ * subtly different semantics (#1439):  - **`mode === 'ban'`** (default,
+ * backward-compat) — the iframe is embedded inside the "Ban Added" success
+ * dialog on `admin.bans.php`. A ban row was JUST inserted; the per-server
+ * loop's job is to (a) record which server actually executed the kick on the
+ * just-created ban row (`UPDATE :prefix_bans SET sid`), and (b) tell the
+ * player they've been banned via the rcon kick message so they know to visit
+ * the panel for appeals. - **`mode === 'kick'`** — the iframe loaded
+ * standalone from the right-click context menu on the public servers page
+ * (`?p=servers` via `web/scripts/server-context-menu.js`). There is NO ban
+ * row; the operator just wants to kick the player without banning. We MUST
+ * skip the `:prefix_bans` UPDATE here — without `mode`, the UPDATE would
+ * silently re-attribute ANY of the player's existing active bans (`RemovedBy
+ * IS NULL`) to whatever server happened to be the kick target, corrupting the
+ * audit trail (#1439 secondary impact). The rcon kick message is also wrong
+ * for this flow — the player isn't banned and CAN rejoin, so "You have been
+ * banned…" lies to them (#1439 user-reported symptom).  Unrecognised `mode`
+ * values are coerced to `'ban'` (backward-compat — old callers that don't
+ * supply the param keep working).
  *
  * @typedef {Object} ApiKickitKickPlayerRequest
  * @typedef {{ status: 'kicked'|'not_found'|'no_connect', hostname: string, ip?: string, port?: string, sid: number, num: number }} ApiKickitKickPlayerResponse

web/scripts/api-contract.js

@@ -354,6 +354,21 @@
             // around what's effectively a "fan a single command out
             // to every enabled server and close" interaction.
             //
+            // The `&mode=kick` query param (#1439) tells the
+            // chromeless kickit page (and the JSON action it fans out
+            // through) that this is a kick-only flow — no ban row
+            // exists. The handler then (a) skips the
+            // `:prefix_bans` UPDATE that would otherwise re-attribute
+            // any of the player's existing active bans to whatever
+            // server happened to answer first, and (b) emits the
+            // "You have been kicked from this server" rcon message
+            // instead of the post-ban "You have been banned by this
+            // server, check $domain for more info" (which would lie
+            // to a player who's actually free to rejoin — the
+            // user-reported #1439 symptom). The post-ban flow on
+            // `admin.bans.php` deliberately doesn't pass `mode`, so
+            // it falls through to the default 'ban' branch.
+            //
             // Ban / Block both route through the panel-chromed
             // smart-default URLs (`?p=admin&c=bans&section=add-ban&steam=…&type=0`
             // / `?p=admin&c=comms&steam=…&type=0`) because they
@@ -384,7 +399,7 @@
             menu.appendChild(buildRow({
                 label: 'Kick player',
                 icon: 'log-out',
-                href: 'pages/admin.kickit.php?check=' + encodeURIComponent(steamid) + '&type=0',
+                href: 'pages/admin.kickit.php?check=' + encodeURIComponent(steamid) + '&type=0&mode=kick',
                 testid: 'context-menu-kick',
             }));