Repository navigation
fix(1441): kick the player from their server when the ban is added - #1459
Merged
Merged
Conversation
Right-clicking a player → Ban → submitting the form would land the ban row in the database (and SourceMod's on-connect check would correctly reject subsequent reconnects), but the player's existing session stayed alive until they voluntarily disconnected — often 20-40 minutes later on a low-turnover server. The user-visible symptom from #1441: "ban is added to database and marked as active but player is not kicked from server". Root cause was a v1.x → v2.0 cutover oversight. The post-`Actions.BansAdd` `.then()` handler in `page_admin_bans_add.tpl` carried: if (r.data && r.data.kickit && typeof window.ShowKickBox === 'function') { window.ShowKickBox(r.data.kickit.check, r.data.kickit.type, ...); } `window.ShowKickBox` was defined in `web/scripts/sourcebans.js`, deleted with the rest of the v1.x bulk JS at #1123 D1 (v2.0.0). The `typeof ... === 'function'` guard silently resolves to `false`, the body of the `if` never runs, AND no console error fires — so the surrounding success branch falls through to a generic "Ban added" toast while the load-bearing rcon side-effect (kick the player off every server they're currently on) silently doesn't happen. This is the worst class of UI regression: chrome reads as success while the action never fires. `api_bans_add` has been returning the kickit envelope shape (`{kickit: {check, type}, reload: true}`) the whole time — only the client-side branch that consumes it was dead. SourceMod's on-connect hook DOES catch the next time the player tries to reconnect, which masked the bug to anyone testing the flow quickly (the "ban then click Kick" two-step the reporter describes is what muscle-memory has been compensating with). Fix --- Mirror the existing comms.add → blockit.php iframe pattern that has been working continuously since v2.0. In the `.then()` handler: if (r && r.ok && r.data && r.data.kickit) { var iframe = document.createElement('iframe'); iframe.id = 'srvkicker'; iframe.style.display = 'none'; iframe.src = 'pages/admin.kickit.php?check=' + encodeURIComponent(r.data.kickit.check) + '&type=' + encodeURIComponent(r.data.kickit.type); document.body.appendChild(iframe); ... } The iframe's `page_kickit.tpl` script (untouched server-side) auto-fires `Actions.KickitLoadServers` on the iframe's load event, enumerates enabled servers, and fans out per-server `Actions.KickitKickPlayer` rcon calls. The operator never sees the iframe (`display: none`) — they just see the "Ban Added" toast followed by a 2s reload of the parent page (matching comms.add's shape exactly). Also standardised the fallback branch's toast title from 'Ban added' (sentence case) to 'Ban Added' (Title Case) so both branches are visually symmetric AND match comms.add's 'Block Added'. The mixed casing was pre-existing but invisible because only the fallback branch ever ran on the live panel; with both branches now running on different installs (kickit-enabled vs kickit-disabled), the inconsistency would have become operator-visible. Coverage -------- - `web/tests/e2e/specs/flows/ban-kickit-iframe.spec.ts` (NEW) pins both branches end-to-end. Stubs `Actions.BansAdd` AND `pages/admin.kickit.php` at the network layer so the spec stays robust under multi-worker DB pressure against the shared `sourcebans_e2e` DB; asserts the iframe is spawned EXACTLY ONCE (`toBe(1)` per reviewer Finding #1 — a future double-mount regression from a `.then()` chain that resolves twice or a `dispatchEvent` re-firing the submit handler must surface here, `toBeGreaterThanOrEqual(1)` would silently pass). Sister test asserts the kickit-DISABLED response (no `r.data.kickit`) does NOT spawn the iframe. - `web/tests/e2e/specs/flows/admin-ban-lifecycle.spec.ts` + `web/tests/e2e/pages/SubmitBanFlow.ts` widen the post-add toast matcher to `/ban added/i` so both casings round-trip; comments document the new contract. - `web/themes/default/page_kickit.tpl` docblock updated to describe the new caller chain (the kickit iframe is now reached from `page_admin_bans_add.tpl`'s success branch as well as the existing context-menu top-level navigation added at #1433). AGENTS.md --------- - New anti-pattern entry banning `typeof window.<LegacyHelper> === 'function'` guards against deleted v1.x helpers (`ShowKickBox`, `TabToReload`) with canonical file paths, the JS field-name asymmetry callout (`r.data.kickit.check` vs `r.data.block.steam` — the URL parameter `check=` is symmetric, the envelope field names are not, because `api_bans_add` and `api_comms_add` chose different envelope shapes), and the regression-guard pointer. Search anchor for future sweeps: `rg "typeof window\.\w+ === 'function'" web/themes/`. - New "Where to find what" row documenting the bans.add → kickit.php iframe fan-out contract alongside the comms.add → blockit.php sibling, the field-name asymmetry reminder, and the regression-guard pointers (`ban-kickit-iframe.spec.ts` + `kickit-iframe.spec.ts` + `KickitTest.php`). Adversarial-reviewer follow-up ------------------------------ A spawned reviewer subagent enumerated 11 findings; #1-#5 (this PR's correctness + docs polish) are addressed: 1. Iframe-request count assertion tightened to `toBe(1)`. 2. AGENTS.md anti-pattern reworded — pre-fix it claimed the new branch was "byte-for-byte symmetric to comms.add"; it isn't (different envelope field names). Now spells out the JS field-name asymmetry as documented structural API drift. 3. Fallback toast title standardised to Title Case. 4. New "Where to find what" row added (this commit). 5. Anti-pattern entry now cites canonical file paths (`page_admin_bans_add.tpl`, `page_admin_comms_add.tpl`, `ban-kickit-iframe.spec.ts`, `kickit-iframe.spec.ts`, `KickitTest.php`). #6-#9 (reload-on-success asymmetry, iframe-load-error UX, parent-reload vs iframe-rcon-fan-out race, iframe-never-removed) are pre-existing comms.add behaviour the PR inherits unchanged. Reviewer's recommendation is to file them as follow-up issues; they're architectural observations, not blockers for #1441's scope. #10 (duplicate `srvkicker` id) and #11 (theoretical XSS via `k.check`) were analysed and confirmed safe by design — three defence-in-depth gates: server-side `SteamID::HANDLER_STRICT_REGEX`, client-side `encodeURIComponent`, Smarty auto-escape. Fixes: #1441 CLA: I have read the CLA Document and I hereby sign the CLA Co-authored-by: Fishy <fishyfishy118@gmail.com>
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.
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 #1441 — right-clicking a player → Ban → submitting the form would land the ban row in the database (and SourceMod's on-connect check would correctly reject subsequent reconnects), but the player's existing session stayed alive until they voluntarily disconnected — often 20-40 minutes later on a low-turnover server.
Root cause was a v1.x → v2.0 cutover oversight. The post-
Actions.BansAdd.then()handler inpage_admin_bans_add.tplcarried:window.ShowKickBoxwas defined inweb/scripts/sourcebans.js, deleted with the rest of the v1.x bulk JS at #1123 D1 (v2.0.0). Thetypeof ... === 'function'guard silently resolves tofalse, the body never runs, AND no console error fires — the surrounding success branch falls through to a generic "Ban added" toast while the load-bearing rcon kick silently doesn't happen. The worst class of UI regression: chrome reads as success while the load-bearing action never fires.api_bans_addhas been returning the kickit envelope shape ({kickit: {check, type}, reload: true}) the whole time — only the client-side branch that consumes it was dead. SourceMod's on-connect hook catches the next reconnect attempt, which masked the bug to anyone testing the flow quickly.Fix
Mirror the existing
comms.add→pages/admin.blockit.phpiframe pattern that has been working continuously since v2.0:page_admin_bans_add.tpl(this PR): replace the deadShowKickBoxbranch with a hidden<iframe id="srvkicker">pointing atpages/admin.kickit.php?check=…&type=…. The iframe'spage_kickit.tplscript (untouched, server-side) auto-firesActions.KickitLoadServerson load, enumerates enabled servers, and fans out per-serverActions.KickitKickPlayerrcon calls. Operator never sees the iframe (display: none) — they just see the "Ban Added" toast followed by a 2s reload of the parent page.'Ban added'(sentence case) to'Ban Added'(Title Case) so both branches match each other ANDcomms.add's'Block Added'. The mixed casing was pre-existing but invisible because only the fallback ever ran on the live panel.Regression coverage
web/tests/e2e/specs/flows/ban-kickit-iframe.spec.ts(NEW)Actions.BansAddANDpages/admin.kickit.phpat the network layer so the spec stays robust under multi-worker DB pressure against the sharedsourcebans_e2eDB; asserts the iframe is spawned exactly once (toBe(1)per reviewer Finding #1 — a future double-mount regression from a.then()chain that resolves twice or adispatchEventre-firing the submit handler must surface here,toBeGreaterThanOrEqual(1)would silently pass). Sister test asserts the kickit-DISABLED response (nor.data.kickit) does NOT spawn the iframe.web/tests/e2e/specs/flows/admin-ban-lifecycle.spec.ts+web/tests/e2e/pages/SubmitBanFlow.ts/ban added/iso both casings round-trip; comments document the new contract.web/themes/default/page_kickit.tplpage_admin_bans_add.tpl's success branch joins the existing #1433 context-menu top-level navigation as the second caller).AGENTS.mdupdates:typeof window.<LegacyHelper> === 'function'guards against deleted v1.x helpers (ShowKickBox,TabToReload) with canonical file paths, the JS field-name asymmetry callout (r.data.kickit.checkvsr.data.block.steam— the URL parametercheck=is symmetric, the envelope field names are not, becauseapi_bans_addandapi_comms_addchose different envelope shapes), and the regression-guard pointer. Search anchor for future sweeps:rg "typeof window\.\w+ === 'function'" web/themes/.bans.add→kickit.phpiframe fan-out contract alongside thecomms.add→blockit.phpsibling, the field-name asymmetry reminder, and the regression-guard pointers.Adversarial-reviewer follow-up
A spawned reviewer subagent enumerated 11 findings. #1-#5 are addressed in this PR:
toBe(1).'Ban Added').page_admin_bans_add.tpl,page_admin_comms_add.tpl,ban-kickit-iframe.spec.ts,kickit-iframe.spec.ts,KickitTest.php).#6-#9 (reload-on-success asymmetry, iframe-load-error UX, parent-reload vs iframe-rcon-fan-out race, iframe-never-removed) are pre-existing
comms.addbehaviour the PR inherits unchanged. Reviewer's recommendation is to file them as follow-up issues; they're architectural observations, not blockers for #1441's scope.#10 (duplicate
srvkickerid) and #11 (theoretical XSS viak.check) were analysed and confirmed safe by design — three defence-in-depth gates: server-sideSteamID::HANDLER_STRICT_REGEX, client-sideencodeURIComponent, Smarty auto-escape; the iframes never coexist in the same document.Test plan
./sbpp.sh phpstan— 0 errors./sbpp.sh ts-check— exit 0./sbpp.sh test --filter='Toast|BanAdd|Kickit'— 30 tests, 133 assertions, all green./sbpp.sh e2e --workers=1 specs/flows/ban-kickit-iframe.spec.ts— 2 chromium passing./sbpp.sh e2e --workers=1 specs/flows/admin-ban-lifecycle.spec.ts— 1 chromium passing (mobile-chromium skipped per project default)workers: 1) passes with no new failures (the 2 pre-existing flakes confirmed unrelated:server-refresh-debounce.spec.tsandcolor-scheme.spec.tspass in isolation, both untouched by this PR per git history)CLA: I have read the CLA Document and I hereby sign the CLA