fix(bans): keep validated SteamID-of-record on IP-type bans; hide synthetic community id (sbpp#1486) (sbpp#1488)

rumblefrog · web-flow · commit 7e4875a71969 · 2026-05-29T03:47:31.000Z
* fix(drawer): hide synthetic community id on IP-type ban detail (sbpp#1486) An IP-type ban stores an empty authid (the SteamID field is intentionally dropped when the operator picks IP type — see testAddIpTypeAlwaysWritesEmptyAuthid). api_bans_detail computes community_id in SQL straight off authid, so an empty authid collapsed the arithmetic to the base 76561197960265728 (STEAM_0:0:0) and the player-detail drawer painted a bogus "Community" id. Gate community_id on the same SteamID::isValidID() check that already guards steam_id / steam_id_3, returning '' so the drawer hides the row. Mirror the gate in api_comms_detail for parity and to cover malformed or empty legacy authids (sbpp#900). No wire-format change for valid authids, so existing snapshots and the api-contract stay valid. * feat(bans): keep validated SteamID-of-record on IP-type bans (sbpp#1486) When an operator fills both the Steam ID and IP fields and picks an IP-type ban, store the SteamID alongside the IP instead of dropping it. The schema has always had separate ip + authid columns; enforcement stays IP-only (the SourceMod plugin matches an IP ban on the ip column alone), so the recorded authid is a record-of-fact surfaced by the ban detail / banlist. The sbpp#1423 validate-before-convert shape gate is preserved on the IP-type branch: a non-empty Steam ID is shape-checked before toSteam2() so garbage returns a structured validation error on the steam field instead of 500ing (the sbpp#1420/sbpp#1423 bug class) or being silently swallowed (the pre-sbpp#1486 drop). An empty Steam ID field records nothing. Covers api_bans_add (JSON) and admin.edit.ban.php (page handler) so the two surfaces can't diverge. Updates the sbpp#1486 drawer guard comment (now covers no-steam IP bans + legacy malformed authids), rewrites the contract test (testAddIpTypeAlwaysWritesEmptyAuthid -> testAddIpTypeKeepsValidatedSteamOfRecord), and reverses the IP-type empty-authid contract + anti-pattern in AGENTS.md in lockstep. * test(bans): fix IP-type steam-of-record newline case (sbpp#1486) The handler trims `$rawSteam` before the shape gate, so a trailing newline (`STEAM_0:0:1\n`) is stripped to a valid `STEAM_0:0:1` and kept, not rejected. Replace the bogus trailing-newline rejection assertion with a non-trimmable mid-string newline (the shape the handler gate actually catches) and add a positive case documenting the trim-then-keep behavior. Aligns AGENTS.md wording with the trim nuance.
diff --git a/AGENTS.md b/AGENTS.md
@@ -833,9 +833,15 @@ of the diff ship together or not at all.
   `ApiError('validation', …, 'steam')` envelope (each test now
   includes the `STEAM_0:0:1\n` / `[U:1:1]\n` / `76561197960265728\n`
   newline-bypass cases as #1423 follow-up #4 regression guards);
-  `web/tests/api/BansTest.php::testAddIpTypeAlwaysWritesEmptyAuthid`
-  pins the IP-type empty-`authid` contract (valid Steam input +
-  garbage + newline-bypass all write empty); the kickit / blockit
+  `web/tests/api/BansTest.php::testAddIpTypeKeepsValidatedSteamOfRecord`
+  pins the #1486 IP-type Steam-of-record contract (valid Steam input
+  is kept alongside the IP, no Steam input writes empty, garbage +
+  non-trimmable malformed shapes — mid-string `\n` — are rejected with
+  a `validation` envelope on the `steam` field BEFORE any row is
+  written, while a trailing-newline `STEAM_0:0:1\n` trims to a valid
+  `STEAM_0:0:1` and is kept; the shape gate still runs on the IP-type
+  branch so a junk value can't 500 or get stored); the
+  kickit / blockit
   `SteamID::compare()` pre-`isValidID()` gate is pinned by
   `KickitTest::testKickPlayerReturnsNotFoundForMalformedSteamId` /
   `testKickPlayerReturnsNotFoundForMalformedIp` and
@@ -859,20 +865,25 @@ of the diff ship together or not at all.
      the operator's raw input on the bounce so they see exactly
      what they typed and can correct the typo without re-typing
      everything else).
-   - `admin.edit.ban.php` ON IP-TYPE bans hard-codes
-     `$_POST['steam'] = ''` regardless of whatever the operator
-     typed in the Steam ID field — the column is the *steam id*
-     of the banned player; on an IP-type ban there is no steam id,
-     so the canonical value is the schema's `NOT NULL default ''`
-     empty string (matching `api_bans_add`'s same-PR fix). The
-     pre-#1423-follow-up-#4 shape (the `82e8c3d2` "canonicalise on
-     IP-type" nit) preserved the canonical-on-valid case but
-     failed to suppress the raw-on-invalid case AND continued
-     writing the canonicalised SteamID into `:authid` for IP-only
-     bans — both shapes are wrong. The form-side input remains
-     visible on the IP-type bounce path (re-emit through the
-     template `placeholder`, NOT a stale value), but the DB write
-     is divorced from it.
+   - `admin.edit.ban.php` ON IP-TYPE bans keeps a *validated* Steam
+     ID-of-record when the operator filled both fields (#1486 — parity
+     with `api_bans_add`'s IP-type branch), and writes the schema's
+     `NOT NULL default ''` empty string when the Steam ID field was
+     left blank. Enforcement stays IP-only (the SourceMod plugin
+     matches an IP ban on the `ip` column alone), so the stored authid
+     is inert plugin-side; it exists so the ban detail / banlist can
+     show which account the IP belonged to. The validate-before-convert
+     gate is still load-bearing: a non-empty Steam ID is run through
+     `SteamID::isValidID()` BEFORE `toSteam2()` so a garbage value
+     bounces with `$validationErrors['steam']` (raw input preserved on
+     the bounce, same as the Steam-branch typo path) instead of
+     escaping the converter as a 500 page render. Pre-#1486 the branch
+     hard-cleared `$_POST['steam'] = ''` regardless of input, dropping
+     a SteamID the operator deliberately typed; #1486 reversed that to
+     keep-when-valid. (The older `82e8c3d2` "canonicalise on IP-type"
+     nit had a worse shape still — it stored a *canonicalised* SteamID
+     but failed to suppress the raw-on-invalid case; #1486's
+     validate-then-convert ladder fixes both.)
    - `page.submit.php` doesn't call `SteamID::toSteam2()` at all —
      it stores the raw user-input verbatim in `:prefix_submissions`
      and the moderation queue resolves the canonical form on
@@ -4336,37 +4347,43 @@ contributions without contacting every contributor individually.
   + `BansTest.php` + `AdminsTest.php` (the
   `"STEAM_0:0:1\n"` / `"[U:1:1]\n"` / `"76561197960265728\n"`
   cases pin the wire-side behavior).
-- Storing the operator-typed Steam ID in `:prefix_bans.authid`
-  on an IP-type ban (the `82e8c3d2` "canonicalise valid IDs on
-  IP-type bans to match pre-tighter behaviour" nit shape) → the
-  `:authid` column is the *steam id* of the banned player. On
-  an IP-type ban (`BanType::Ip = 1`) there is no steam id, by
-  definition; the canonical "no steam id" value is the schema's
-  `NOT NULL default ''` empty string. The `82e8c3d2` nit aimed
-  to preserve "pre-tightening behavior" for the case where the
-  operator typed a valid-looking SteamID into the form's
-  Steam ID box and then flipped the type radio to "IP-type" —
-  pre-tightening the unconditional `toSteam2($rawSteam)` would
-  have stored the canonicalised value in `:authid` AND the
-  unconditional run would have raised on `garbage` and 500'd
-  the page. The nit canonicalised the valid case (good
-  intention) but didn't suppress the storage path (the bug it
-  carried), AND failed to defend the invalid-input branch on
-  the IP-type arm (the 500 was still reachable via
-  `?type=1&steam=garbage`). The right shape is: hard-code
-  `$_POST['steam'] = ''` for `$banType === BanType::Ip` and
-  let the operator's form input remain visible only as
-  client-side echo (template `placeholder`, NOT a stale value).
-  Matches the `api_bans_add` write-side fix in the same PR
-  (`$steam = $banType === BanType::Ip ? '' : ...`). The
-  asymmetry pre-#1423-follow-up-#4 (page handler stored
-  canonicalised, JSON handler stored empty) was its own bug
-  class — third-party callers POSTing to the iframe-routed
-  page handler vs. the JSON dispatcher produced inconsistent
-  DB state for the same logical input. Regression guard:
-  `web/tests/api/BansTest.php::testAddIpTypeAlwaysWritesEmptyAuthid`
-  (valid Steam input + garbage + newline-bypass all write
-  empty `authid` on `type=1`).
+- Storing an UNVALIDATED operator-typed Steam ID in
+  `:prefix_bans.authid` on an IP-type ban → as of #1486 an IP-type
+  ban DOES keep a Steam ID-of-record when the operator fills both
+  fields (the schema has always had separate `ip` + `authid`
+  columns; enforcement stays IP-only because the SourceMod plugin
+  matches an IP ban on the `ip` column alone, so the stored authid
+  is inert plugin-side and exists only so the ban detail / banlist
+  can show which account the IP belonged to). What stays forbidden
+  is storing it WITHOUT the shape gate: the value MUST pass
+  `SteamID::isValidID()` (page handler) / `HANDLER_STRICT_REGEX`
+  (JSON handler) BEFORE `toSteam2()`, exactly like the Steam-type
+  branch. Skipping the gate re-opens two bug classes — (a) a junk
+  value (`?type=1&steam=garbage`) escapes `toSteam2()` as
+  `Exception('Invalid SteamID input!')` → 500 (the #1420 / #1423
+  follow-up #4 class), and (b) a malformed authid lands on disk and
+  the drawer / banlist later derive a synthetic `community_id`
+  (`STEAM_0:0:0`) off it (#1486's display bug). The right shape is
+  the validate-then-convert ladder: empty Steam ID → write `''`
+  (nothing recorded); non-empty → shape-gate → `toSteam2()` → store;
+  bad shape → `validation` envelope (JSON) / `$validationErrors['steam']`
+  bounce with raw input preserved (page handler). Both surfaces
+  (`api_bans_add` + `admin.edit.ban.php`) share the ladder so the
+  JSON dispatcher and the iframe-routed page handler can't diverge
+  on the same logical input. The pre-#1486 shape hard-cleared
+  `authid` for `type=1`, silently dropping a SteamID the operator
+  deliberately typed; the older `82e8c3d2` "canonicalise on IP-type"
+  nit stored a *canonicalised* value but failed to suppress the
+  raw-on-invalid path — both are superseded by the gated keep.
+  Regression guard:
+  `web/tests/api/BansTest.php::testAddIpTypeKeepsValidatedSteamOfRecord`
+  (valid Steam input kept alongside the IP, empty input writes
+  empty `authid`, garbage + non-trimmable malformed shapes — mid-string
+  `\n` — rejected with a `validation` envelope on the `steam` field
+  before any row is written, trailing-newline trims to a valid value
+  and is kept) +
+  `web/tests/integration/SteamIDValidationOrderTest.php` (pins the
+  validate-before-convert order in `admin.edit.ban.php`).
 - Calling `SteamID::compare($a, $b)` (or any other
   `SteamID::*` method that funnels through `toSteam64()` /
   `resolveInputID()`) with operator-controlled input that
diff --git a/web/api/handlers/bans.php b/web/api/handlers/bans.php
@@ -71,19 +71,29 @@ function api_bans_add(array $params): array
         if (!preg_match(SteamID::HANDLER_STRICT_REGEX, $rawSteam)) {
             throw new ApiError('validation', 'Please enter a valid Steam ID or Community ID', 'steam');
         }
+    } elseif ($rawSteam !== '') {
+        // #1486: IP-type ban where the operator ALSO filled the Steam ID
+        // field. Keep it as a record-of-fact alongside the IP instead of
+        // silently dropping the input. Enforcement stays IP-only — the
+        // SourceMod plugin matches an IP ban purely on the `ip` column
+        // (sbpp_main.sp: `(type=0 AND authid…) OR (type=1 AND ip…)`), so
+        // this authid is inert plugin-side; it exists only so the ban
+        // detail / banlist can show which account the IP belonged to.
+        //
+        // The shape gate is still load-bearing: an unvalidated
+        // `toSteam2('garbage')` throws `Invalid SteamID input!` and the
+        // dispatcher's `Throwable` fallback turns it into a 500 envelope
+        // (the #1420 / #1423 follow-up #4 bug class). Validate before
+        // convert so the recorded id can't be corrupt and the add can't
+        // 500. An empty Steam ID field on an IP ban is fine (no record).
+        if (!preg_match(SteamID::HANDLER_STRICT_REGEX, $rawSteam)) {
+            throw new ApiError('validation', 'Please enter a valid Steam ID or Community ID', 'steam');
+        }
     }
-    // For IP-typed bans the `:authid` column is the *steam id*, of which
-    // there is none — write empty string regardless of whatever the
-    // caller passed in `$rawSteam`. Pre-#1423 follow-up #4 the handler
-    // converted any non-empty `$rawSteam` here without re-running the
-    // shape gate (which was Steam-branch-only), so a hostile / typo'd
-    // caller passing `type=1&steam=garbage&ip=1.2.3.4` triggered
-    // `toSteam2('garbage')` → `Exception('Invalid SteamID input!')` →
-    // `Api::handle` `Throwable` fallback → 500 envelope (the bug class
-    // #1420 was supposed to close, surfacing on the IP-type branch the
-    // original review didn't cover). The page-handler sibling
-    // (`admin.edit.ban.php`) carries the matching write-side fix.
-    $steam = $banType === BanType::Ip ? '' : ($rawSteam === '' ? '' : SteamID::toSteam2($rawSteam));
+    // Steam bans require a SteamID; IP bans keep it only when one was
+    // typed (empty otherwise). The shape gate above guarantees this
+    // conversion cannot throw on either branch.
+    $steam = $rawSteam === '' ? '' : SteamID::toSteam2($rawSteam);
     if (empty($ip) && $banType === BanType::Ip) {
         throw new ApiError('validation', 'You must type an IP', 'ip');
     }
@@ -891,10 +901,21 @@ function api_bans_detail(array $params): array
         $state = 'active';
     }
 
+    // An IP-type ban carries a SteamID only when the operator typed one
+    // (#1486 made it a record-of-fact); otherwise authid is empty. Some
+    // legacy rows also hold malformed authids (#900). Gate steam2 on the
+    // shape check so toSteam3() / the community id below never derive a
+    // synthetic value from an empty or junk authid.
     $steam2 = $authid !== '' && SteamID::isValidID($authid) ? $authid : '';
-    // Some legacy rows hold malformed authids (#900); fall back to a
-    // canonical placeholder so toSteam3() doesn't blow up the response.
     $steam3 = $steam2 !== '' ? (string)SteamID::toSteam3($steam2) : '';
+    // #1486: community_id is computed in SQL straight off BA.authid, so an
+    // empty authid (IP-type ban with no recorded SteamID) or a malformed
+    // one collapses the arithmetic to the base 76561197960265728
+    // (STEAM_0:0:0) — the bogus "Community" id the drawer used to paint.
+    // Gate it on the same validity check as steam2/3 so it surfaces ONLY
+    // when there's a real SteamID behind it (Steam bans, or IP bans where
+    // the operator recorded one).
+    $communityId = $steam2 !== '' ? (string)$row['community_id'] : '';
 
     $removedByName = null;
     if ($row['RemovedBy'] !== null && (int)$row['RemovedBy'] > 0 && !$hideAdmin) {
@@ -942,7 +963,7 @@ function api_bans_detail(array $params): array
             'type'         => $type,
             'steam_id'     => $steam2,
             'steam_id_3'   => $steam3,
-            'community_id' => (string)$row['community_id'],
+            'community_id' => $communityId,
             'ip'           => $hideIps || $banIp === '' ? null : $banIp,
             'country'      => !empty($row['country']) && trim((string)$row['country']) !== '' ? (string)$row['country'] : null,
         ],
diff --git a/web/api/handlers/comms.php b/web/api/handlers/comms.php
@@ -503,6 +503,13 @@ function api_comms_detail(array $params): array
 
     $steam2 = $authid !== '' && SteamID::isValidID($authid) ? $authid : '';
     $steam3 = $steam2 !== '' ? (string)SteamID::toSteam3($steam2) : '';
+    // #1486: community_id is computed in SQL straight off C.authid. A
+    // comm block can't be IP-typed, but a malformed/empty legacy authid
+    // (#900) collapses the arithmetic to the base 76561197960265728
+    // (STEAM_0:0:0). Gate it on the same validity check as steam2/3 so the
+    // drawer never paints a synthetic "Community" id (parity with
+    // api_bans_detail).
+    $communityId = $steam2 !== '' ? (string)$row['community_id'] : '';
 
     $removedByName = null;
     if ($row['RemovedBy'] !== null && (int)$row['RemovedBy'] > 0 && !$hideAdmin) {
@@ -553,7 +560,7 @@ function api_comms_detail(array $params): array
             'name'         => (string)$row['name'],
             'steam_id'     => $steam2,
             'steam_id_3'   => $steam3,
-            'community_id' => (string)$row['community_id'],
+            'community_id' => $communityId,
             // Comm rows don't store an IP — `:prefix_comms` has no `ip`
             // column. Always null; the field is only here so the
             // drawer's `renderOverviewPane` can read `player.ip`
diff --git a/web/pages/admin.edit.ban.php b/web/pages/admin.edit.ban.php
@@ -143,22 +143,16 @@ function emitEditBanToastAndRedirect(string $kind, string $title, string $body,
     // pass. Empty input is its own error message; non-empty-but-bad
     // is "please enter a valid…".
     //
-    // The IP-type branch writes `:authid = ''` regardless of whatever
-    // happened to be in the Steam ID input. The column is the *steam
-    // id* of the banned player; on an IP-type ban there is no steam
-    // id, so the canonical value is the schema's `NOT NULL default ''`
-    // empty string — matching the API handler (`api_bans_add`'s
-    // INTENT pre-#1423 follow-up #4, fully enforced after that PR)
-    // and matching the v1.x library's "throw on garbage" outcome
-    // (pre-tightening the unconditional `toSteam2($rawSteam)` would
-    // have raised on `garbage` and the page died with a 500; storing
-    // user-typed garbage into `:authid` is a regression introduced
-    // by the 82e8c3d2 "canonicalise valid IDs on IP-type bans" nit
-    // that preserved the canonical-on-valid case but failed to
-    // suppress the raw-on-invalid case). The form-side input remains
-    // visible on the IP-type bounce path (re-emit through the
-    // `placeholder` on the template, NOT a stale value), but the DB
-    // write is divorced from it.
+    // #1486: the IP-type branch keeps the Steam ID as a record-of-fact
+    // when the operator filled both fields, instead of dropping it. The
+    // SourceMod plugin enforces an IP ban on the `ip` column ONLY
+    // (sbpp_main.sp: `(type=0 AND authid…) OR (type=1 AND ip…)`), so the
+    // stored authid is inert plugin-side; it exists so the ban detail /
+    // banlist can show which account the IP belonged to. The shape gate
+    // still runs before `toSteam2()` so a garbage value can't escape as
+    // an uncaught Exception (500 page render) — same validate-then-convert
+    // ladder as the Steam branch, mirrored in `api_bans_add`. An empty
+    // Steam ID field on an IP ban records nothing.
     if ($postBanType === BanType::Steam) {
         if ($rawSteam === '') {
             $error++;
@@ -178,14 +172,22 @@ function emitEditBanToastAndRedirect(string $kind, string $title, string $body,
             // through the shared `ID_PATTERNS` table.
             $_POST['steam'] = \SteamID\SteamID::toSteam2($rawSteam);
         }
-    } else {
-        // IP-type ban: clear the steam slot for the DB write below
-        // (`:authid = $_POST['steam']` rides this). Whatever the
-        // operator happened to type in the Steam ID field stays
-        // visible in `$rawSteam` for any client-side echo, but it
-        // does NOT land in the DB. See the block comment above for
-        // the schema rationale.
+    } elseif ($rawSteam === '') {
+        // IP-type ban, no Steam ID typed: nothing to record.
         $_POST['steam'] = '';
+    } elseif (!\SteamID\SteamID::isValidID($rawSteam)) {
+        // IP-type ban with a typed Steam ID-of-record: validate its shape
+        // before converting so a garbage value can't escape `toSteam2()`
+        // as an uncaught Exception. Bounce with the raw input preserved,
+        // same as the Steam-branch typo path above.
+        $error++;
+        $validationErrors['steam'] = 'Please enter a valid Steam ID or Community ID';
+        $_POST['steam'] = $rawSteam;
+    } else {
+        // Keep the canonical Steam ID alongside the IP (record-of-fact;
+        // enforcement is IP-only). Parity with `api_bans_add`'s IP-type
+        // branch.
+        $_POST['steam'] = \SteamID\SteamID::toSteam2($rawSteam);
     }
 
     if ($error === 0 && empty($_POST['ip']) && $postBanType === BanType::Ip) {
diff --git a/web/tests/api/BansTest.php b/web/tests/api/BansTest.php
diff --git a/web/tests/api/CommsTest.php b/web/tests/api/CommsTest.php
