Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rumblefrog merged 1 commit into from
May 25, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions AGENTS.md
View file
Open in desktop

Large diffs are not rendered by default.

130 changes: 117 additions & 13 deletions web/api/handlers/kickit.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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']];
}
Expand All @@ -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';
}
15 changes: 15 additions & 0 deletions web/includes/View/KickitView.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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',
) {
}
}
13 changes: 13 additions & 0 deletions web/pages/admin.kickit.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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('{');
Expand Down
20 changes: 19 additions & 1 deletion web/scripts/api-contract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 16 additions & 1 deletion web/scripts/server-context-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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&name=…`
// / `?p=admin&c=comms&steam=…&type=0&name=…`) because they
Expand Down Expand Up @@ -402,7 +417,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',
}));

Expand Down
Loading
Loading