fix(bans): keep validated SteamID-of-record on IP-type bans; hide synthetic community id (#1486)#1488
Merged
Merged
Conversation
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 (#900). No wire-format change for valid authids, so existing snapshots and the api-contract stay valid.
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 #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 #1420/#1423 bug class) or being silently swallowed (the pre-#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 #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.
rumblefrog
changed the title
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1486. The report: ban a player by IP while also filling the Steam ID field, and the ban detail drawer rendered a bogus
STEAM_0:0:0"Community" id.Two parts, one per commit:
fix(drawer)— hide the synthetic community id.community_idis computed in SQL straight offBA.authid. An empty (or malformed)authidcollapsed the arithmetic to the base76561197960265728(STEAM_0:0:0).api_bans_detail/api_comms_detailnow gatecommunity_idon the sameSteamID::isValidID()check already guardingsteam_id/steam_id_3, so it surfaces only when there's a real SteamID behind it.feat(bans)— keep the SteamID instead of dropping it. Root-cause of the user's follow-up ("I don't think we should drop the SteamID if both are filled out — is this a schema limitation?"). It was not a schema limitation::prefix_banshas always had separateip+authidcolumns. The handler was hard-clearingauthidfortype=1. Now, when the operator fills both fields on an IP-type ban, the SteamID is kept as a record-of-fact alongside the IP.Why keeping it is safe
Enforcement stays IP-only. The SourceMod plugin matches an IP ban on the
ipcolumn alone (sbpp_main.sp:(type=0 AND authid…) OR (type=1 AND ip…)), so the storedauthidis inert plugin-side. It exists purely so the ban detail / banlist can show which account the IP belonged to. This also removes a pre-existing asymmetry: the handler already retained a typed IP on a Steam-type ban, but dropped a typed SteamID on an IP-type ban.Security gate preserved
The #1420 / #1423-follow-up-#4 validate-before-convert contract is intact. A non-empty Steam ID on an IP ban is shape-gated (
HANDLER_STRICT_REGEXin the JSON handler,SteamID::isValidID()in the page handler) beforetoSteam2(), so a garbage value (?type=1&steam=garbage, newline-bypass, etc.) returns a structuredvalidationerror on thesteamfield rather than escaping the converter as a 500 or being silently swallowed. Empty Steam ID field → nothing recorded.Changes
web/api/handlers/bans.php—api_bans_addkeeps a validated SteamID on IP bans;api_bans_detailcommunity-id guard + comment.web/pages/admin.edit.ban.php— same keep-when-valid behavior via the validate-then-convert ladder; rationale comment updated.web/tests/api/BansTest.php—testAddIpTypeAlwaysWritesEmptyAuthid→testAddIpTypeKeepsValidatedSteamOfRecord(kept-on-valid, empty-on-blank,validation-rejected on garbage/newline with no row written); detail test asserts the IP still surfaces.AGENTS.md— reversed the IP-type empty-authidcontract + anti-pattern in lockstep (the residual anti-pattern is storing an unvalidated SteamID, not storing one at all).Test plan
BansTest,CommsTest,SteamIDValidationOrderTest), PHPStan, API-contract, E2E.php -lclean on all modified PHP files.SteamIDValidationOrderTestvalidate-before-convert order preserved inadmin.edit.ban.php(firstisValidIDprecedes firsttoSteam2).STEAM_0:0:0.authid, drawer shows IP only.