fix(admin): rewire dead sourcebans.js helpers across admin surfaces (#1402) - #1408
Merged
Conversation
…1402) Sweeps the post-#1397 cluster of admin surfaces where the click chain dead-ended at a helper deleted with `web/scripts/sourcebans.js` at #1123 D1. Same shape as #1397's `RemoveMod` fix (silent / loud `ReferenceError` on every click), applied to the rest of the cluster: - **Mods → Add new MOD → Submit** (`ProcessMod`): the form's `onsubmit="return ProcessMod()"` referenced a v1.x helper that validated input + called `sb.api.call(Actions.ModsAdd)`. Replaced with an inline page-tail dispatcher that intercepts `submit`, runs the same client-side gates (name + URL + icon non-empty), then `sb.api.call(Actions.ModsAdd, …)` per the `page_admin_groups_add.tpl` reference shape. `setBusy` flips the three-attribute busy contract during the in-flight call. - **Mods → Upload icon popup** (`window.opener.icon(...)`): `UploadHandler::handle()` emits `<script>window.opener.icon(filename)</script>` on successful upload, but `window.icon` was undefined in both `page_admin_mods_add.tpl` (new-mod flow) and `page_admin_edit_mod.tpl` (edit-mod flow), so the popup never closed and the icon filename never landed on the parent's `#icon_hid` hidden input. Both templates now define `window.icon = function (filename) { … }` inside a `{literal}…{/literal}` block that patches the hidden input + updates the visible affordances (preview / "Choose file" label). Same shape `window.demo` uses on the ban pages. - **Admins → Add new admin → Submit** (`ProcessAddAdmin`): same shape as `ProcessMod`. Intercept submit, validate (username + password + SteamID + server-group + web-group), then `sb.api.call(Actions.AdminsAdd)`. `event.preventDefault()` blocks the native fallback POST so a JSON failure no longer races a full-page reload. - **Admins → Add → Generate password button** (`LoadGeneratePassword`): rewired to `sb.api.call(Actions.AdminsGeneratePassword)`; success fills both `password` + `confirm` inputs in place. No CSRF surface (read-only call). - **Admins → Add → server / web permissions `<select>` reveal** (`update_server` / `update_web`): the conditional dependent-input reveal (Custom permissions → flag picker; New admin group → new-name + SM flags) is pure client-side DOM. Replaced the two dead helpers with `data-action="adminadd-update-server" / "adminadd-update-web"` `<select>` change handlers that toggle the right `hidden` attributes per the original v1.x semantics. - **Comms → Edit block → submit with validation error** (`window.addEvent('domready', …)`): the MooTools DOMready wrapper in `$errorScript` referenced an undefined global, so the validation-error toast never painted. Per scope guardrails on this issue, I dropped the MooTools wrapper and replaced its body with vanilla `document.addEventListener('DOMContentLoaded', …)` calling `window.SBPP.showToast` directly. The matching `$('id').innerHTML` / `setStyle` calls inside the `changeReason()` helper became `document.getElementById('id').textContent` / `el.style.display = 'block'`. The broader sweep of `<script>ShowBox(...)</script>` toast blobs across other pages stays for #1403. - **Bans → Group ban — URL submit + bulk-from-friends** (`LoadGroupBan` / `ProcessGroupBan` / `CheckGroupBan`): chained through a page-tail dispatcher in `admin.bans.php` that picks up `data-action="groupban-submit" / "groupban-bulk-submit"` and walks the legacy two-step `Actions.BansGroupBan` → `Actions.BansBanMemberOfGroup` chain. The first step parses the URL into a group name, the second enumerates + bans members. Bulk path tracks the `last` checkbox sentinel so a 20-group bulk-ban only fires one success toast. No new `Actions.GroupbanCheck` was needed — `bans.group_ban` IS the URL-parse step `LoadGroupBan` used to perform first. - **Bans → Group ban → Tick select-all** (`TickSelectAll`): rewired to `data-action="groupban-select-all"` (covers both the table-header button + the "Select all" link below). Toggle state is computed from the `chkb_<n>` checkboxes' live state per the v1.x semantics (any-unchecked → check all; all-checked → uncheck all). - **Banlist / Commslist / admin.bans comment editor trash** (`RemoveComment`): the four call sites (page.banlist.php, page.commslist.php, admin.bans.php protests + submissions) used to inline the same dead helper. Replaced with a single shared `web/scripts/comment-actions.js` dispatcher loaded from `core/footer.tpl` (`<script src="./scripts/comment-actions.js" defer>`). Each trigger emits `data-action="comment-delete"` + `data-cid="<int>"` + `data-ctype="<B|C|S|P>"` + `data-page="<int>"`; the dispatcher `window.confirm`s the destructive intent, then `sb.api.call(Actions.BansRemoveComment, { cid, ctype, page })`. The `ctype` letter matches `:prefix_comments.type` (B=ban, C=comm-block, S=submission, P=protest); `api_bans_remove_comment`'s `ctype` arm consumes all four. Single mount point is the contract — don't duplicate inline per page. E2E regression coverage ----------------------- Four new specs under `web/tests/e2e/specs/flows/`: - `mods-add-form.spec.ts` — happy-path submit + missing-icon client-side gate. Stubs `Actions.ModsAdd` via `page.route` to assert the wire format without mutating the e2e DB. - `admins-add-form.spec.ts` — happy-path submit, Generate-password button fills both password inputs, "New admin group" / "Custom permissions" reveals the right dependent inputs. Pinned to chromium (form-shaped; mobile would just burn CI minutes). - `comment-delete-dispatcher.spec.ts` — `data-action="comment-delete"` trigger fires `Actions.BansRemoveComment` with the right `cid` / `ctype` / `page`, `confirm()` cancel suppresses the API call, and the dispatcher loads globally (the `<script src="./scripts/comment-actions.js" defer>` include is in `core/footer.tpl`). - `groupban-dispatcher.spec.ts` — empty URL → inline error + no API call, valid URL → chains `Actions.BansGroupBan` → `Actions.BansBanMemberOfGroup`, `data-action="groupban-select-all"` toggle flips synthetic `chkb_<n>` checkboxes. The group-ban surface ships behind `config.enablegroupbanning`, which `data.sql` defaults to 0. The spec flips it on in `beforeAll` and reverts in `afterAll` via a new `setSettingE2e(key, value)` helper in `fixtures/db.ts` that shells out to `web/tests/e2e/scripts/set-setting-e2e.php` (mirror of the `REPLACE INTO sb_settings` shape `BansTest.php` uses for the same reason). AGENTS.md updates ----------------- - Extended the existing `onclick="<Helper>()"` / `onclick="if (typeof <Helper> === 'function') …"` anti-pattern bullet to cross-reference #1402 and enumerate the cluster (`ProcessMod`, `ProcessAddAdmin`, `LoadGeneratePassword`, `update_server` / `update_web`, `LoadGroupBan` / `ProcessGroupBan` / `CheckGroupBan` / `TickSelectAll`, `RemoveComment`, `window.opener.icon(...)`, `window.addEvent('domready', …)`). - Added a new bullet for the MooTools `window.addEvent('domready', …)` DOMready idiom (silent no-op every time it surfaced since #1123 D1; replace with `document.addEventListener('DOMContentLoaded', …)` or drop the wrapper outright when the script tag lands after the elements it touches). - New "Where to find what" rows for: - Wiring a `window.opener.<callback>(...)` slot on a parent template (the icon / demo / mapimg callback shape). - Wiring a comment-delete trash icon (the shared `comment-actions.js` dispatcher). - Flipping a `:prefix_settings` row from an E2E spec (`setSettingE2e` helper + its caller-cleanup contract). Test plan --------- - ./sbpp.sh phpstan ✓ - ./sbpp.sh test ✓ (677 tests) - ./sbpp.sh ts-check ✓ - ./sbpp.sh composer api-contract ✓ (no diff) - ./sbpp.sh e2e --workers=1 ✓ (279 passed) - ./sbpp.sh e2e --grep "1402|mods-add|admins-add|comment-delete|LoadGroupBan" --workers=1 ✓ (12 passed) Out of scope (deferred per the issue body) ----------------------------------------- - `admin.admins.php` `$serverscript` blob (sister #1404, parallel worktree). - `admin.groups.php` `LoadServerHostPlayersList` echo (sister #1404). - The remaining `<script>ShowBox(...)</script>` toast blobs on lostpassword / protest / banlist / commslist / admin.edit.comms (sister #1403).
Second commit on the same branch addressing the eight defects the adversarial review surfaced after the initial #1402 rewire landed. Kept as a separate commit so the audit trail stays visible. HIGH (must-fix) --------------- 1. Owner-flag privilege escalation through the rewired Add-admin form. The pre-existing api_admins_add handler had no HasAccess(WebPermission::Owner) check (vs api_admins_edit_perms which does), and the rewire exposed the OWNER checkbox to every admin with ADMIN_ADD_ADMINS — full panel takeover with one click. * api_admins_add: mirror api_admins_edit_perms's guard at the top of the handler (Log::add + Api::redirect to no_access). Covers both the `wg='c'` (direct mask) and `wg='n'` (new group inherits mask) escalation paths. * AdminAdminsAddView: new `can_grant_owner` View prop. * admin.admins.php: pass `$userbank->HasAccess(WebPermission::Owner)`. * page_admin_admins_add.tpl: gate the OWNER checkbox with `{if \$can_grant_owner}`. * web/tests/api/AdminsTest.php: two new tests — testAddBlocksGrantingOwnerWithoutOwner (non-owner is redirected, no row landed) + testAddAllowsGrantingOwnerForOwner (positive case so the guard isn't over-zealous). 2. Add-admin success path silently dropped Actions.SystemRehashAdmins. The handler returns `rehash` in the envelope (sid CSV) — the legacy ProcessAddAdmin chained it; the rewrite navigated away without firing. config.enableadminrehashing defaults to '1', so without the chain a new admin can log in to the panel but can't moderate on game servers until the next restart. * page_admin_admins_add.tpl: thread `data.rehash` into a SystemRehashAdmins call before the navigate timer. Mirrors _admin_edit_helpers.php:fireRehash's .then/.catch shape. * admins-add-form.spec.ts: new e2e arm that stubs both Actions.AdminsAdd (returning rehash:'1,2') and Actions.SystemRehashAdmins, asserts the call sequence and that servers=1,2 was forwarded. 3. Stale flags from hidden #web-flags-block / #server-flags rode into Actions.AdminsAdd after a dropdown flip. updateWeb / updateServer toggled `hidden` but left checkbox state + text-input values intact; collectWebFlags walked the unscoped #web-flags-block. Repro: "Custom permissions" → tick Owner → "No permissions" → submit → mask: ADMIN_OWNER. Second route to accidental OWNER grant. * page_admin_admins_add.tpl: updateServer / updateWeb now clear dependent state (uncheck flag checkboxes, clear name inputs, clear srv flags string) when the dropdown swings back to a non-revealing value. collectWebFlags is also scoped to `:not([hidden])` as defense-in-depth on top of the clear; sibling collectServerFlags / collect*NewName helpers ride the same hidden-ancestor guard. * admins-add-form.spec.ts: new e2e arm flips the dropdown through the trap and asserts the API receives mask: 0. MEDIUM (concerns) ----------------- 4. Missing .catch() arms on every new sb.api.call chain — sb.api.call doesn't reject on network failures (it synthesises an envelope), but a throw escaping the success callback would leave the button stuck in setBusy forever. Per AGENTS.md "Loading state on action buttons" — setBusy(btn, false) on every non-navigating response branch. * page_admin_admins_add.tpl: AdminsAdd + AdminsGeneratePassword. * page_admin_mods_add.tpl: ModsAdd (sibling surface, same rewire wave). * admin.bans.php loadGroupBan: both outer BansGroupBan and inner BansBanMemberOfGroup. * comment-actions.js: BansRemoveComment. 5. Generated password left visible (type='text') indefinitely. The rewrite flipped both password fields to type=text "to match v1.x UX", but v1.x LoadGeneratePassword never changed .type. Privacy / shoulder-surf / screenshot leak. * page_admin_admins_add.tpl: keep the fields at type='password' (matching legacy). * admins-add-form.spec.ts: update the type assertion to pin 'password' (was 'text'). LOW (nits) ---------- 6. Drop unused data-action="groupban-submit-form" attribute on page_admin_bans_groups.tpl's <form> — no dispatcher wires it. 7. Drop page.waitForTimeout(200/300) negative-assertion settles from three e2e specs (comment-delete / groupban / mods-add). The inline error renders / dispatcher returns synchronously — once the positive state is visible, the negative assertion can land. 8. Drop `defer` from the body-tail comment-actions.js include. `defer` is a no-op on a script that lives at the body tail (the parser is already past the body), so the markup matched what the runtime did, just less misleadingly. Reviewer-flagged concerns kept out of scope (preserved from origin/main, not introduced by this PR): the delcomlink permission asymmetry (Owner-only delete vs. anyone-can-edit-their-own) and the per-template `window.icon` callback duplication. Both tracked separately if they need addressing. Quality gates (all green at workers=1): * PHPStan: 240/240 files, no errors * PHPUnit: 679 tests, 2703 assertions * ts-check: clean * API contract: regenerated, no diff * E2E: 187 passed / 100 skipped (mobile), no failures
Conflicts: - AGENTS.md: auto-merged, no manual resolution (both #1402 and #1404 added rows in the same anti-patterns block; git's three-way merger correctly interleaved them). - web/includes/View/AdminAdminsAddView.php: drop the `server_script` ctor param (gone at #1404), keep `can_grant_owner` (added during #1402's adversarial review for the owner-flag privilege escalation defense-in-depth). - web/pages/admin.admins.php: drop `server_script:` from the AdminAdminsAddView() call, keep `can_grant_owner:`. The pre-loop `$serverscript` variable wasn't built on this branch anyway (it went at #1404's first commit) so removing the kwarg is a pure syntactic resolution. - web/themes/default/page_admin_admins_add.tpl: replace the legacy `{$server_script nofilter}` echo with #1404's explanatory `{* *}` comment block, keep #1402's massive new submit-handler wiring block in full (lines 338+).
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
Closes #1402.
The v2.0 cutover deletion of
web/scripts/sourcebans.js(#1123 D1) left ~10 admin surfaces with dead JS handlers. This PR rewires them tosb.api.call(Actions.PascalName)or vanilla DOM following the #1352 canonical confirm-dialog shape.Wired surfaces:
Actions.ModsDelete), Add (Actions.ModsAdd), Edit (icon uploadwindow.iconcallback)Actions.AdminsAdd+ pairedSystemRehashAdmins), Generate password, perm-mode selects (updateServer/updateWeb)Actions.BansGroupBan→BansBanMemberOfGroupchain), Tick select-allActions.BansRemoveCommentvia sharedcomment-actions.jsdispatcher)window.addEvent('domready', …)form-field error highlights → vanilla DOM +window.SBPP.showToastSecurity-hardening in the same PR (caught by adversarial review):
api_admins_addnow rejectsmask & ADMIN_OWNERfrom non-owners (mirrorsapi_admins_edit_permsfrom origin/main; pre-fix the deadProcessAddAdminmasked this)can_grant_ownergate on the OWNER checkbox so non-owners don't see the affordanceupdateServer/updateWebnow clear dependent state when parent goes back to "no permissions", preventing stale-flag ride-throughLoading-state contract: every new
sb.api.callbutton goes throughwindow.SBPP.setBusy(...);.catch()arm on every chain so a network error doesn't leave the button stuck.Test plan
testAddBlocksGrantingOwnerWithoutOwner,testAddAllowsGrantingOwnerForOwner, plus the existingAdminsTestcoverage)--workers=1:mods-add-form.spec.ts(form-success / icon-upload callback / validation)admins-add-form.spec.ts(perm-toggle clear, owner-gate hide, success → SystemRehashAdmins chain)comment-delete-dispatcher.spec.ts(shared dispatcher fires per-site)groupban-dispatcher.spec.ts(URL submit / bulk-from-friends / select-all)Related
sourcebans.js