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

[rumblefrog]

Summary

Eight commits — the implementer's original fix (e3a8106d), an
adversarial-review follow-up (821e81f4) closing a server-side
bypass surface the original fix left open, three further follow-ups
(fd81750b + 9555f4f8 + 82e8c3d2) that incorporate the three
"out of scope" items from the original review into this same PR,
and four second-round review fixes (2f1fe61e + da583d31 +
fccb177b + 6a018c44) that closed gaps the first-round review
missed (per-handler regex drift on the D modifier, IP-type ban
:authid storage divergence, unvalidated input to SteamID::compare()
in kickit/blockit, an overly permissive OpenID regex in
SteamAuthHandler, and a missing D modifier in the install
wizard's Steam-ID gate).

The user-visible bug. Per issue #1420 (reporter: iBoonie), the
comms-add form silently swallowed every invalid SteamID. The form's
inline tail script wired the submit button to a legacy ProcessBan()
helper that called into sb.message.show against the v1.x
#dialog-placement chrome shell — DOM ids the v2.0 theme doesn't
render anywhere. Net effect: every error branch silently no-op'd
client-side, and the server-side SteamID::toSteam2() call threw a
generic \Exception that the dispatcher's catch-all wrapped as a
500 with body "An unexpected error occurred" the chrome treated as
a server outage, not a per-field validation failure.

Commits

• e3a8106d fix(comms): surface SteamID validation errors on the comms-add form (#1420)
• 821e81f4 fix(comms): close substring-bypass via strict SteamID regex (review #1420)
• fd81750b fix(steamid): tighten SteamID library regexes (review #1420 follow-up #1)
• 9555f4f8 fix(steamid): validate-before-convert across page-handler form-POST surfaces (review #1420 follow-up #2)
• 82e8c3d2 fix(steamid): canonicalise valid Steam IDs on IP-type bans (#1420 follow-up #2 nit)
• 2f1fe61e fix(steamid): unify handler regex + write empty authid on IP-type bans (#1420 review follow-up)
• da583d31 fix(steamid): pre-validate inputs to SteamID::compare in kickit / blockit (#1420 review follow-up)
• fccb177b fix(steamid): tighten SteamAuthHandler OpenID regex + install wizard pattern (#1420 review follow-up)
• 6a018c44 test(steamid): cover trim-bypass shapes in admins / comms add tests (#1420 review follow-up)

Follow-up #1: tighten SteamID::isValidID() itself (fd81750b)

The reviewer flagged web/includes/SteamID/SteamID.php::isValidID
as structurally unsafe: regexes were unanchored, used [0|1]
character classes (where | is a literal pipe, not alternation),
and used \d* (accepts zero digits). The follow-up:

• Lifted the four accepted shapes into a shared SteamID::ID_PATTERNS
  constant. Both isValidID() and resolveInputID() now consume
  the same table, so they cannot drift again. Patterns:
  • ^STEAM_[01]:[01]:\d+$D — Steam2
  • ^\[U:1:\d+\]$D — Steam3 bracketed
  • ^U:1:\d+$D — Steam3 bracketless (preserved compat)
  • ^\d{17}$D — Steam64
• The D modifier closes the STEAM_0:0:1\n newline-bypass
  sibling (without D, PHP's $ matches end-of-string OR a final
  \n).
• Verified the library is in-tree under web/includes/SteamID/ (NOT
  vendor/) — the project owns it, the "third-party-vendored"
  framing in the original review was incorrect.
• Audited every caller (rg 'SteamID::' web/ game/ docker/):
  • Install wizard, admin.edit.* page handlers, page.submit.php,
    page.protest.php, page.banlist.php, page.commslist.php,
    SteamAuthHandler.php — all unaffected (their happy paths
    produce inputs the strict regex still accepts).
  • SourceMod plugins extract SteamIDs from GetClientAuthId and
    bind directly to MySQL queries; the PHP library is panel-side
    only.
  • The three JSON *_add handlers KEEP their per-handler preg_match
    as defence-in-depth (both layers agree on the accepted shape;
    the library is one refactor away from someone "simplifying" the
    shared table back to a loose form, so the per-handler gate
    catches that regression).
• Added web/tests/integration/SteamIDValidationTest.php (104
  tests / 183 assertions) — drives every Comms block steamID validation - No notification on invalid steamID #1420 bypass shape +
  loose-character-class variants + empty / whitespace / embedded
  IDs / newline-bypass, plus ID_PATTERNS introspection and the
  PHP-8.x int-input deprecation guard.

Follow-up #2: page-handler form-POST surfaces (9555f4f8 + 82e8c3d2)

The four form-POST surfaces that called SteamID::toSteam2()
BEFORE validation had the same bug class as the JSON handlers,
but with a different failure mode: the converter's
Exception('Invalid SteamID input!') escaped the page handler
unhandled, the chrome's PageDie() never fired, and the user
got a generic 500 page render instead of the inline per-field
error on the form. Follow-up #1's library tightening made the
throw stricter — which made the 500 page render strictly MORE
frequent on every surface that hadn't been swept yet.

Surfaces swept:

• web/pages/admin.edit.ban.php — validate-then-convert
  ladder, error into $validationErrors['steam'], raw input
  preserved on bounce.
• web/pages/admin.edit.comms.php — same ladder, error into
  $errorFields[] = ['steam.msg', '…'].
• web/pages/admin.edit.admindetails.php — same ladder, error
  into $validationErrors['steam']; the $resolvedSteam /
  $steamIsValidShape distinction surfaces empty vs invalid as
  distinct messages.
• web/pages/page.submit.php — already gated on isValidID();
  the library tightening closes the bypass without a handler edit.
  The legacy STEAM_0: empty-sentinel pre-fill (which would have
  failed the new template pattern="…" check) was dropped.
• web/pages/admin.bans.php's importBans branch — discovered
  during the audit, same bug class. A single malformed banid …
  line in the operator's uploaded banned_user.cfg aborted the
  entire import mid-file (no transaction wrapper) and the operator
  had no signal as to which line broke. Validate-before-convert
  skips and counts malformed lines; the success toast surfaces the
  skipped count alongside the imported count.

Template-side changes (matching the JSON-flow add-form templates
from the original #1420 PR):

• All four form templates carry the strict
  pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}" + actionable
  title="…" attributes on the Steam ID input.
• page_admin_edit_admins_details.tpl + page_admin_edit_comms.tpl
  also carry required + aria-required="true" (every admin row
  must carry an :authid; every comm-block must target a
  SteamID).
• page_admin_edit_ban.tpl + page_submitban.tpl are intentionally
  NOT required — both legitimately accept an empty Steam ID
  when the operator provides an IP instead.
• page_submitban.tpl dropped novalidate so native validation
  actually fires client-side; the cross-field "one of Steam ID OR
  IP" guard stays in JS (HTML can't express it natively).

Follow-up #3: lift shared helper for the two add-form IIFEs — considered, deferred

The reviewer asked whether the two add-form IIFEs (page_admin_comms_add.tpl
and page_admin_bans_add.tpl) could be consolidated into a shared
web/scripts/admin-add-form.js helper. After diffing the two
IIFEs in detail:

Shared (~35 lines, ~25% of each IIFE):

• The api() / actions() / $id() / setMsg() / toast() /
  setBusy() helper functions are identical byte-for-byte
  (graceful-degradation wrappers for when theme.js is stripped
  by a third-party theme).

Divergent (~75% of each IIFE, fundamentally different logic):

• Validation strategy: comms-add uses native form.checkValidity()
  (every input is unconditionally required); bans-add uses an
  ad-hoc validate(type) function for type-conditional steam-vs-IP
  validation (native HTML can't express the type-conditional
  gate).
• Field harvest: comms-add: 5 fields; bans-add: 9 fields
  (adds ip, dfile, dname, fromsub).
• Success path: comms-add builds an iframe to
  admin.blockit.php to fire RCON; bans-add calls
  ShowKickBox/TabToReload + resets the form in place.
• Error routing: comms-add routes error.field to specific
  *.msg divs (steam.msg / nick.msg / reason.msg); bans-add
  just toasts.
• Post-success behavior: comms-add keeps button busy +
  page-reloads; bans-add re-enables button + form.reset().

Why deferred:

1. The 75% divergence is exactly where the logic lives. Lifting
   the 25% boilerplate would force a parameterised helper API
   (validate callback, harvest callback, onSuccess callback,
   onErrorField map) that would leak the divergence through
   the helper's parameter shapes — replacing 35 lines of
   duplication with 50+ lines of indirection that's harder to
   read than the inlined IIFEs.
2. AGENTS.md's "Loading state on action buttons" convention
   explicitly says inline page-tail scripts should define
   setBusy(btn, busy) as a local wrapper that delegates to
   window.SBPP.setBusy when present and falls back to plain
   disabled otherwise. The fallback is what keeps the
   double-click gate working on third-party themes that strip
   theme.js. Lifting the wrapper to a shared file at
   web/scripts/admin-add-form.js would fight that contract
   (a third-party theme stripping theme.js would also strip
   the new shared script).
3. Both forms are in active flux (just edited in this PR for
   validation); locking them into a shared API now would make
   future per-form tweaks more painful.
4. Zero user-visible benefit. The reviewer explicitly flagged
   this as "Not blocking; the two forms now both fail closed
   on invalid input via the native browser popover + the
   server-side strict regex."

The reviewer's heuristic — "If the result is meaningfully MORE
complex or LESS readable than the two inlined IIFEs (a common
refactor anti-pattern), STOP and report back that #3 is
'considered, not landed'" — applies here. Filing this as
deferred rather than forced.

AGENTS.md updates

• Rewrote the "SteamID inputs" Conventions block to reflect the
  library tightening (the preg_match is now true
  defence-in-depth alongside SteamID::isValidID()).
• Added a "page-handler form-POST surfaces" subsection
  documenting the validate-then-convert ladder shape across
  admin.edit.* / page.submit.php / importBans.
• Six Anti-patterns entries added across both review rounds:
  • Loosening SteamID::ID_PATTERNS (back to substring matcher
    / [0|1] / \d*).
  • Removing the per-handler preg_match "now that the library
    is strict too".
  • Calling SteamID::toSteam2() before SteamID::isValidID()
    on a page handler's raw POST input (the convert-before-
    validate bug class).
  • Wrapping toSteam2() in try/catch (\Exception) as a
    substitute for the upstream isValidID() gate.
  • Hand-rolling the strict SteamID-shape regex literal at the
    per-handler preg_match call site (now sourced from
    SteamID::HANDLER_STRICT_REGEX).
  • Storing the operator-typed Steam ID in :prefix_bans.authid
    on an IP-type ban (must be '' per the schema's
    NOT NULL DEFAULT '').
  • Calling SteamID::compare(…) with operator-controlled input
    that hasn't been gated through SteamID::isValidID() first.
  • Tightening SteamAuthHandler::validate()'s OpenID-claimed-ID
    regex to anything OTHER than 7\d{16}+ with the D modifier.

Second-round review (2f1fe61 + da583d3 + fccb177 + 6a018c4)

A second adversarial review surfaced four gaps the first-round
fixes didn't fully close. All four landed in this PR.

Finding 1 — Handler regex drift on the D modifier (2f1fe61e)

The first-round fix kept hand-rolled preg_match literals at each
handler call site (admins.php / bans.php / comms.php) as "defence in
depth". But the three copies subtly DIFFERED from the library's
ID_PATTERNS (and from each other) on the D modifier — without
it, STEAM_0:0:1\n matches the handler's ^…$ anchors (because $
accepts end-of-string OR just-before-final-\n in the default mode)
and then fails the library's SteamID::isValidID() (which DOES carry
D post-#1423 follow-up #1), triggering SteamID::toSteam2() to
throw, which the dispatcher's Throwable fallback wraps as a generic
500 envelope. Exactly the bug class #1420 was supposed to close.

Lifted to a single library constant SteamID::HANDLER_STRICT_REGEX
with paired tests (SteamIDValidationTest::testHandlerStrictRegex*)
and a source-of-truth pin
(SteamIDValidationOrderTest::testJsonHandlersUseSingleSourceOfTruthRegex).

Finding 2 — IP-type ban :authid divergence (2f1fe61e)

The first-round nit (82e8c3d2) "canonicalised valid Steam IDs on
IP-type bans to match pre-tighter behaviour". The pre-tighter
behaviour was itself a bug: the page handler wrote whatever the user
typed in the Steam ID input into :authid regardless of the ban
being IP-typed. The nit fix preserved the canonical-on-valid case
but failed to suppress the raw-on-invalid case — garbage on an
IP-type ban still landed in :authid verbatim.

The JSON API handler (api_bans_add) also disagreed with the page
handler: api converted $rawSteam for every non-empty input
regardless of $banType, so type=1&steam=garbage&ip=1.2.3.4 ran
toSteam2('garbage') → 500 envelope (the bug class #1420 was
supposed to close, surfacing on the IP-type branch the original
review didn't cover).

Both halves now converge on :authid = '' for IP-type bans, matching
the schema's NOT NULL DEFAULT ''. The form-side input remains
visible on bounce, but the DB write is divorced from it. Test:
BansTest::testAddIpTypeAlwaysWritesEmptyAuthid (three subtests
covering valid Steam-shape, garbage, and trailing-newline bypass).

Finding 3 — Unvalidated input to SteamID::compare() in kickit/blockit (da583d31)

api_kickit_kick_player and api_blockit_block_player iterate the
A2S GetPlayers list and call SteamID::compare($a, $b) on the
operator-supplied check parameter for each returned player. Pre-fix
the operator-controlled check went straight into compare() →
resolveInputID() → throws on any malformed shape → dispatcher's
Throwable fallback → generic 500. Same bug class #1420 was
supposed to close, on a different handler pair the first-round
review didn't audit.

Fix: pre-validate via SteamID::isValidID() (Steam arm) or
filter_var($check, FILTER_VALIDATE_IP) (kickit's IP arm) and
return the structured status: 'not_found' envelope the handlers
already emit for "no matching player". Tests:
KickitTest::testKickPlayerReturnsNotFoundForMalformedSteamId,
testKickPlayerReturnsNotFoundForMalformedIp, and the matching
BlockitTest::testBlockPlayerReturnsNotFoundForMalformedSteamId.

Finding 4 — SteamAuthHandler OpenID regex + install wizard D modifier (fccb177b)

SteamAuthHandler::validate()'s extract regex was 7[0-9]{15,25}+
— wider than reality (Steam in practice always returns exactly
17-digit Steam64s for individual accounts, and the library's
ID_PATTERNS carries ^\d{17}$D). Pre-fix a 16-digit OR 18-25-digit
OpenID claim slipped past validate() but then failed
SteamID::toSteam2() in check(); the exception escaped the
constructor's call site unhandled (LightOpenID's mid-flow redirect
leaves no chrome to catch it), so the operator landed on a 500
mid-Steam-login round-trip — silent failure mode, no toast.

Tightened to 7\d{16}+ with the D modifier so the two gates agree
byte-for-byte. Added a SteamID::isValidID() belt-and-suspenders
gate in check() so any future drift on either side surfaces as a
clean redirect-to-login-failed.

web/install/pages/page.5.php (the install wizard's initial-admin
form) carried its own regex without the D modifier. Tightened to
/^STEAM_[01]:[01]:[0-9]+$/D so the wizard rejects newline-bypassed
input at the same gate the panel runtime does. Tests:
SteamIDValidationTest::testSteamAuthHandlerOpenIdRegexAcceptsOnly17DigitsStartingWith7
and testInstallWizardSteamIdRegexCarriesDModifier.

Finding 5 — Handler trim-bypass test gaps (6a018c44)

The first-round handler tests covered trailing-newline cases
("STEAM_0:0:1\n" etc.), but the _register.php dispatcher
trim()s string params upstream of the handler — those test cases
were quietly redundant. Replaced with shapes trim() does NOT
defend against: mid-string newlines ("STEAM_0:0:1\nGARBAGE") and
trailing non-breaking spaces ("STEAM_0:0:1\xC2\xA0", U+00A0 — trim
only handles ASCII whitespace). Library-side coverage in
SteamIDValidationTest::testHandlerStrictRegexRejectsNewlineBypass
remains (the library doesn't trim, so the newline-bypass tests
there are non-redundant).

Test plan

• ./sbpp.sh phpstan — 241/241 OK
• ./sbpp.sh test — 856 tests, 3252 assertions OK
• ./sbpp.sh composer api-contract — no diff (no handler signatures changed)
• ./sbpp.sh ts-check — clean
• CI=1 ./sbpp.sh e2e --grep 'comms-add|admin-edit-comms|admin-edit-ban|submit|protest|admins-add' — 48 passed (matches CI workers=1; the second-round parallel-mode run hit 12 failures that all surfaced as forbidden / No access per the documented Fixture::truncateAndReseed race, gone under workers: 1)

Out of scope / future work

• Splitting :authid storage off the :prefix_bans table into a
  separate :prefix_bans_authids link table so the schema can
  carry a NOT NULL FK without the IP-type-ban shim. Real schema
  cleanup; out of scope for Comms block steamID validation - No notification on invalid steamID #1420.
• Lifting the shared 25%-boilerplate IIFE helper from
  page_admin_comms_add.tpl and page_admin_bans_add.tpl —
  deferred per the analysis above; will land if the two forms
  converge further on validation strategy.

Fixes #1420.