fix(steamid): surface validation errors across comms / bans / admin add + edit forms (#1420)

web/api/handlers/admins.php

@@ -120,7 +120,15 @@ function api_admins_add(array $params): array
     $mask        = (int)($params['mask'] ?? 0);
     $srvMask     = (string)($params['srv_mask'] ?? '');
     $name        = (string)($params['name'] ?? '');
-    $steam       = SteamID::toSteam2((string)($params['steam'] ?? ''));
+    // #1420 — defer `SteamID::toSteam2()` until AFTER the strict-shape
+    // gate below. Pre-fix this line called `toSteam2()` directly on
+    // the raw input; `resolveInputID()` throws a generic `\Exception`
+    // for unrecognised shapes (`'asdf'`, `'12345'`, … the same
+    // garbage the comms / bans handlers ate) which escaped the
+    // handler and surfaced as a 500. See "JSON API" / "SteamID inputs"
+    // in AGENTS.md for the contract; this file is the third reference
+    // shape after `api_comms_add` and `api_bans_add`.
+    $rawSteam    = trim((string)($params['steam'] ?? ''));
     $email       = (string)($params['email'] ?? '');
     $password    = (string)($params['password']  ?? '');
     $password2   = (string)($params['password2'] ?? '');
@@ -167,12 +175,20 @@ function api_admins_add(array $params): array
     if ($userbank->isNameTaken($name)) {
         throw new ApiError('validation', 'An admin with this name already exists', 'name');
     }
-    if (empty($steam)) {
+    if ($rawSteam === '') {
         throw new ApiError('validation', 'You must type a Steam ID or Community ID for the admin.', 'steam');
     }
-    if (!SteamID::isValidID($steam)) {
+    // #1420 — strict anchored regex mirrors the form template's
+    // client-side `pattern` (HTML's `pattern` is implicitly `^…$`),
+    // so a curl-driven caller can't smuggle embedded-substring
+    // garbage past the gate (`'asdf 76561197960265728 garbage'`
+    // matches `SteamID::isValidID`'s unanchored substring regex and
+    // `toSteam2()` then emits a negative-Z-component canonical form
+    // into `:prefix_admins.authid`).
+    if (!preg_match(SteamID::HANDLER_STRICT_REGEX, $rawSteam)) {
         throw new ApiError('validation', 'Please enter a valid Steam ID or Community ID.', 'steam');
     }
+    $steam = SteamID::toSteam2($rawSteam);
     if ($userbank->isSteamIDTaken($steam)) {
         $taken = '';
         foreach ($userbank->GetAllAdmins() as $a) {

web/api/handlers/bans.php

@@ -42,20 +42,56 @@ function api_bans_add(array $params): array
     $rawType  = (int)($params['type'] ?? 0);
     $banType  = BanType::tryFrom($rawType) ?? BanType::Steam;
     $type     = $banType->value;
-    $steam    = SteamID::toSteam2(trim((string)($params['steam'] ?? '')));
+    $rawSteam = trim((string)($params['steam'] ?? ''));
     $ip       = preg_replace('#[^\d\.]#', '', (string)($params['ip'] ?? ''));
     $length   = (int)($params['length'] ?? 0);
     $dfile    = (string)($params['dfile'] ?? '');
     $dname    = (string)($params['dname'] ?? '');
     $reason   = (string)($params['reason'] ?? '');
     $fromsub  = (int)($params['fromsub'] ?? 0);
 
-    if (empty($steam) && $banType === BanType::Steam) {
-        throw new ApiError('validation', 'You must type a Steam ID or Community ID', 'steam');
-    }
-    if ($banType === BanType::Steam && !SteamID::isValidID($steam)) {
-        throw new ApiError('validation', 'Please enter a valid Steam ID or Community ID', 'steam');
+    // #1420 — validate the SteamID shape BEFORE handing it to
+    // `SteamID::toSteam2()`. `resolveInputID()` throws a bare
+    // `\Exception` (not an `ApiError`) on any unrecognised shape;
+    // that escaped to the dispatcher's catch-all and produced a 500
+    // with body "An unexpected error occurred" instead of the
+    // structured `validation`-coded error the chrome's toast can
+    // render.
+    //
+    // The bundled `SteamID::isValidID()` is NOT a sufficient gate on
+    // its own — its regexes are unanchored with loose character
+    // classes, so an embedded-SteamID-with-garbage like
+    // `'asdf 76561197960265728 garbage'` matches the substring AND
+    // `toSteam2()` emits a corrupt canonical form (negative Z
+    // component from the parser eating surrounding bytes) which then
+    // gets bound into `:prefix_bans.authid`. The strict regex below
+    // mirrors the form template's client-side `pattern` attribute
+    // byte-for-byte (HTML's `pattern` is implicitly anchored `^…$`),
+    // so a curl-driven caller can't smuggle garbage past the gate
+    // that the browser's pattern-mismatch popover already rejects
+    // for form users. Comms-add and admin-add carry the same regex;
+    // all three handlers stay in lockstep so a future menu / deep-
+    // link / API client only has to learn one accepted shape.
+    if ($banType === BanType::Steam) {
+        if ($rawSteam === '') {
+            throw new ApiError('validation', 'You must type a Steam ID or Community ID', 'steam');
+        }
+        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));
     if (empty($ip) && $banType === BanType::Ip) {
         throw new ApiError('validation', 'You must type an IP', 'ip');
     }

web/api/handlers/blockit.php

@@ -42,6 +42,24 @@ function api_blockit_block_player(array $params): array
     $type   = (int)($params['type']   ?? 0);
     $length = (int)($params['length'] ?? 0);
 
+    // #1423 follow-up #4 — gate `$check` shape BEFORE the
+    // `SteamID::compare()` call below; the comms-block flow is always
+    // Steam-ID-keyed (there is no "block by IP" path) so the gate is
+    // unconditional. `compare()` routes through `toSteam64()` which
+    // throws on any non-isValidID input; without this gate a hostile
+    // caller posting `?check=garbage` 500s the handler instead of
+    // getting the `not_found` envelope the iframe loop expects.
+    if (!SteamID::isValidID($check)) {
+        return [
+            'status'   => 'not_found',
+            'sid'      => $sid,
+            'num'      => $num,
+            'hostname' => '',
+            'ip'       => '',
+            'port'     => '',
+        ];
+    }
+
     $serverInfo = $GLOBALS['PDO']->query("SELECT ip, port FROM `:prefix_servers` WHERE sid = :sid");
     $GLOBALS['PDO']->bind(':sid', $sid);
     $sdata = $GLOBALS['PDO']->single() ?: ['ip' => '', 'port' => ''];

web/api/handlers/comms.php

@@ -22,16 +22,46 @@ function api_comms_add(array $params): array
     // that Smarty auto-escapes (#1087). Store raw, escape on display.
     $nickname = (string)($params['nickname'] ?? '');
     $type     = (int)($params['type']   ?? 0);
-    $steam    = SteamID::toSteam2(trim((string)($params['steam']  ?? '')));
+    $rawSteam = trim((string)($params['steam'] ?? ''));
     $length   = (int)($params['length'] ?? 0);
     $reason   = (string)($params['reason'] ?? '');
 
-    if (empty($steam)) {
+    // #1420 — validate the SteamID shape BEFORE handing it to
+    // `SteamID::toSteam2()`. The conversion helper calls
+    // `resolveInputID()` internally, which throws a bare `\Exception`
+    // (not an `ApiError`) on any unrecognised shape — that escaped to
+    // the dispatcher's catch-all and surfaced as a generic 500 with
+    // body "An unexpected error occurred. See server logs for
+    // details." instead of the structured `validation`-coded error
+    // shape the chrome's toast can render. The reporter observed it
+    // as "no notification" because the comms add page's tail script
+    // was *also* broken (legacy MooTools `$('id')` selectors against
+    // a global that no longer exists post-#1123 D1), so the API
+    // round-trip never happened — fixing the front-end alone would
+    // have exposed the same 500 the bans-add path also carries.
+    //
+    // The bundled `SteamID::isValidID()` is NOT a sufficient gate on
+    // its own — its regexes are unanchored with loose character
+    // classes (`STEAM_[0|1]:[0:1]:\d*` — note the `|` inside `[...]`
+    // is a literal pipe, not alternation, and the missing `^`/`$`
+    // anchors mean an embedded-SteamID-with-garbage like
+    // `'asdf 76561197960265728 garbage'` matches the substring AND
+    // `toSteam2()` then emits `'STEAM_0:0:-38280598980132864'` (the
+    // negative Z component is the parser eating the surrounding
+    // bytes, the result gets bound into the DB). The strict regex
+    // below mirrors the form template's client-side `pattern`
+    // attribute byte-for-byte (HTML's `pattern` is implicitly
+    // anchored `^…$`), so a curl-driven caller can't smuggle garbage
+    // past the gate that the browser's pattern-mismatch popover
+    // already rejects for form users. Mirror `api_bans_add` /
+    // `api_admins_add` — all three handlers use the same regex.
+    if ($rawSteam === '') {
         throw new ApiError('validation', 'You must type a Steam ID or Community ID', 'steam');
     }
-    if (!SteamID::isValidID($steam)) {
+    if (!preg_match(SteamID::HANDLER_STRICT_REGEX, $rawSteam)) {
         throw new ApiError('validation', 'Please enter a valid Steam ID or Community ID', 'steam');
     }
+    $steam = SteamID::toSteam2($rawSteam);
     if (!in_array($type, [1, 2, 3], true)) {
         throw new ApiError('validation', 'Invalid block type. Must be one of: gag (1), mute (2), or both (3).', 'type');
     }

web/api/handlers/kickit.php

@@ -61,6 +61,40 @@ function api_kickit_kick_player(array $params): array
     $num   = (int)($params['num']   ?? 0);
     $type  = (int)($params['type']  ?? 0);
 
+    // #1423 follow-up #4 — gate the `check` shape BEFORE we reach the
+    // `SteamID::compare()` call below. For `type === 0` (Steam-ID
+    // kick), `compare()` routes through `toSteam64()` → the library
+    // throws `Exception('Invalid SteamID input!')` on any input that
+    // fails the strict shape gate. The exception escapes the handler
+    // via `Api::handle`'s `Throwable` fallback as a generic 500
+    // envelope; the iframe loop then can't tell "no match" apart from
+    // "your input was garbage", and a hostile caller posting
+    // `?check=garbage&type=0` reliably 500s the panel. For
+    // `type === 1` (IP-address kick) we run `filter_var` instead so a
+    // malformed IP returns the standard `not_found` envelope rather
+    // than triggering the SteamID compare on whatever the operator
+    // typed in the wrong box.
+    if ($type === 0 && !SteamID::isValidID($check)) {
+        return [
+            'status' => 'not_found',
+            'sid'    => $sid,
+            'num'    => $num,
+            'hostname' => '',
+            'ip'     => '',
+            'port'   => '',
+        ];
+    }
+    if ($type === 1 && !filter_var($check, FILTER_VALIDATE_IP)) {
+        return [
+            'status' => 'not_found',
+            'sid'    => $sid,
+            'num'    => $num,
+            'hostname' => '',
+            'ip'     => '',
+            'port'   => '',
+        ];
+    }
+
     $serverInfo = $GLOBALS['PDO']->query("SELECT ip, port FROM `:prefix_servers` WHERE sid = :sid");
     $GLOBALS['PDO']->bind(':sid', $sid);
     $sdata = $GLOBALS['PDO']->single() ?: ['ip' => '', 'port' => ''];

web/includes/Auth/Handler/SteamAuthHandler.php

@@ -32,7 +32,23 @@ private function login(): void
 
     private function validate(): string|false
     {
-        $pattern = "/^https:\/\/steamcommunity\.com\/openid\/id\/(7[0-9]{15,25}+)$/";
+        // #1423 follow-up #4 — tightened from `7[0-9]{15,25}+` to
+        // exactly 17 digits (`\d{16}` after the literal `7`) to match
+        // `SteamID::ID_PATTERNS`'s `^\d{17}$D` shape. Pre-fix a
+        // 16-digit OR 18-25-digit OpenID claim slipped past this
+        // regex but then failed `SteamID::toSteam2()` in `check()`
+        // (which routes through the library's `\d{17}` gate), the
+        // exception escaped the constructor unhandled, and the
+        // operator landed on a 500 mid-Steam-login round-trip
+        // (silent failure mode — there's no `try/catch` here and the
+        // chrome's `PageDie()` doesn't run on a callback redirect).
+        // Steam in practice always returns 17-digit Steam64 IDs in
+        // the claimed_id URL; this regex now matches the library
+        // contract byte-for-byte so a future Steam-side change that
+        // emits a 16-digit ID (or a 24-digit one for some hypothetical
+        // future user range) surfaces here as a clean false return
+        // (operator sees the login-failed message), not as a 500.
+        $pattern = "/^https:\/\/steamcommunity\.com\/openid\/id\/(7\d{16}+)$/D";
 
         // Issue #1273: $this->openid->data is $_POST / $_GET (mixed), and
         // PHPStan can't see that LightOpenID::validate() guarantees
@@ -48,6 +64,17 @@ private function validate(): string|false
 
     private function check(string $steamid): void
     {
+        // Defense-in-depth: `validate()` already gates the input
+        // through the strict 17-digit regex, but the library's
+        // `toSteam2()` raises a generic `\Exception` on any input that
+        // fails `isValidID()`. The exception would escape the
+        // constructor's call site unhandled (LightOpenID's mid-flow
+        // redirect leaves no chrome to catch it), so the gate here is
+        // load-bearing belt-and-suspenders.
+        if (!\SteamID\SteamID::isValidID($steamid)) {
+            header("Location: ".Host::complete()."/index.php?p=login&m=steam_failed");
+            return;
+        }
         $steamid = \SteamID\SteamID::toSteam2($steamid);
 
         $this->dbs->query('SELECT aid FROM `:prefix_admins` WHERE authid = :authid');