From 9584b963336d3726969b2b1b765a701aee5b8772 Mon Sep 17 00:00:00 2001 From: rumblefrog Date: Sun, 17 May 2026 17:40:20 -0400 Subject: [PATCH 1/2] fix(admin): rewire dead sourcebans.js helpers across admin surfaces (#1402) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `` 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 `` 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 `` 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_` 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` (`` toast blobs on lostpassword / protest / banlist / commslist / admin.edit.comms (sister #1403). --- AGENTS.md | 50 +- web/pages/admin.bans.php | 240 ++++++++-- web/pages/admin.edit.comms.php | 57 ++- web/pages/page.banlist.php | 16 +- web/pages/page.commslist.php | 8 +- web/scripts/comment-actions.js | 131 ++++++ web/tests/e2e/fixtures/db.ts | 46 ++ web/tests/e2e/scripts/set-setting-e2e.php | 83 ++++ .../e2e/specs/flows/admins-add-form.spec.ts | 203 ++++++++ .../flows/comment-delete-dispatcher.spec.ts | 188 ++++++++ .../specs/flows/groupban-dispatcher.spec.ts | 240 ++++++++++ .../e2e/specs/flows/mods-add-form.spec.ts | 141 ++++++ web/themes/default/core/footer.tpl | 13 + web/themes/default/page_admin_admins_add.tpl | 434 +++++++++++++++++- web/themes/default/page_admin_bans_groups.tpl | 58 ++- web/themes/default/page_admin_edit_mod.tpl | 40 ++ web/themes/default/page_admin_mods_add.tpl | 234 +++++++++- web/themes/default/page_bans.tpl | 5 + 18 files changed, 2078 insertions(+), 109 deletions(-) create mode 100644 web/scripts/comment-actions.js create mode 100644 web/tests/e2e/scripts/set-setting-e2e.php create mode 100644 web/tests/e2e/specs/flows/admins-add-form.spec.ts create mode 100644 web/tests/e2e/specs/flows/comment-delete-dispatcher.spec.ts create mode 100644 web/tests/e2e/specs/flows/groupban-dispatcher.spec.ts create mode 100644 web/tests/e2e/specs/flows/mods-add-form.spec.ts diff --git a/AGENTS.md b/AGENTS.md index 3c9860be1..d44394575 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2171,10 +2171,55 @@ contacting every contributor individually. reason field, and a page-tail script that dispatches to `sb.api.call(Actions.PascalName, …)`. The `Actions.PascalName` shape (NOT a string literal) catches typos at api-contract - regen time. Search anchors for the cleanup sweep: + regen time. **#1402** swept the rest of the post-#1397 cluster + (`ProcessMod`, `ProcessAddAdmin`, `LoadGeneratePassword`, + `update_server` / `update_web`, `LoadGroupBan` / + `ProcessGroupBan` / `CheckGroupBan` / `TickSelectAll`, + `RemoveComment`, `window.opener.icon(...)`, + `window.addEvent('domready', …)`). Constructive form submits + (`ProcessMod` / `ProcessAddAdmin`) intercept the form's `submit` + event, validate, then `sb.api.call(Actions.ModsAdd / .AdminsAdd)` + per the `page_admin_groups_add.tpl` reference. Multi-step + chains (`LoadGroupBan` → `Actions.BansGroupBan` → + `Actions.BansBanMemberOfGroup`) live in a page-tail dispatcher + next to the surface in `web/pages/admin.bans.php`. The trash- + can-on-a-comment trigger (banlist / commslist / admin.bans + protests + submissions) is the rare case where the same + affordance ships on three sibling pages — that one rides a + **shared `web/scripts/comment-actions.js` dispatcher** loaded + from `core/footer.tpl` so the four call sites (page.banlist.php, + page.commslist.php, admin.bans.php protests, admin.bans.php + submissions) share one source of truth instead of four inlined + page-tail blocks. The icon-upload callback (`window.opener.icon(...)` + emitted by `UploadHandler::handle()`) is wired via a per-page + `window.icon = function (filename) { … }` block in the parent + template (`page_admin_mods_add.tpl` + `page_admin_edit_mod.tpl`); + this is the same shape the demo upload uses (`window.demo` on + `admin.bans.php` / `admin.edit.ban.php`). Search anchors for the + cleanup sweep: `rg "typeof \w+ === ['\"]function['\"]" web/themes/` for the guarded shape, and `rg "onclick=\"[A-Z]\w+\(" web/themes/` for the unguarded sister shape. +- `window.addEvent('domready', …)` MooTools DOMready idiom in + inline page-tail blocks (or any `Element.prototype` / + `$$('css selector')` / `el.addEvent(...)` MooTools method call) + → use vanilla `document.addEventListener('DOMContentLoaded', + function () { … })` or — better, since panel templates render + the page-tail script AFTER the elements it targets — drop the + wrapper entirely and run synchronously. MooTools was removed + with `sourcebans.js` at #1123 D1; every `window.addEvent` + callsite that survived was a silent no-op (`window.addEvent` + is undefined, the event listener never registered, the body + never ran). Pre-#1402 `web/pages/admin.edit.comms.php` shipped + a `$errorScript` blob wrapping its DOM operations in + `window.addEvent('domready', ...)`; the validation-error toast + never painted because the wrapper itself threw. Vanilla DOM + access (`document.getElementById('id').value` / + `el.style.display = 'block'`) replaces the `$('id').value` / + `$('id').setStyle(…)` MooTools idioms in the same sweep — both + go in one PR per file because the body of the + `window.addEvent('domready', …)` callback almost always uses + MooTools `$()` too. - `web/scripts/contextMenoo.js` / `sb.contextMenu` / global `AddContextMenu` → removed at #1306. The vanilla shims were back-compat scaffolding for the MooTools-era right-click menu the @@ -2772,10 +2817,12 @@ contacting every contributor individually. | Add or rename a permission | `web/configs/permissions/web.json`, then regen contract | | Render a page | `web/pages/.php` + `web/includes/View/*View.php` | | Add a new edit page in the admin.edit.* cluster (e.g. `admin.edit..php`) | `web/pages/admin.edit..php` (the page handler — thin "validate input, build View, render" shape) + `web/includes/View/AdminEditView.php` (typed View DTO) + `web/themes/default/page_admin_edit_.tpl` (template). Shared helpers live in `web/pages/_admin_edit_helpers.php` (`sbpp_admin_edit_die_with_toast()` for permission / not-found guards, `sbpp_admin_edit_emit_tail_script()` for form-success / validation-error feedback that fires `window.SBPP.showToast()` and writes errors into `.msg` divs, `sbpp_admin_edit_collect_rehash_sids()` for the post-save Rehash Admins step). Anti-patterns to avoid: inline `echo '
...'` blocks, `echo '
…'` banners, MooTools `$('id').value` reads, legacy JS handler names (`ButtonOver`, `ProcessEditAdminPermissions`, etc.) — all swept as part of `goals#5`. CSRF gate every POST via `\CSRF::rejectIfInvalid();` after the `isset($_POST[''])` arm. | +| Wire a `window.opener.(...)` slot on the parent template of a popup file-upload page (e.g. `window.icon`, `window.demo`, `window.mapimg`) | The parent template defines `window. = function (filename) { … }` inside an inline `` and the call throws `TypeError: window.opener.icon is not a function` — popup never closes, the parent form's hidden input never updates, the uploaded asset is orphaned on disk. The `UploadHandler` emits the call unconditionally; the parent's job is to be ready for it. | | Add a popup file-upload page (demo / icon / mapimage / new asset type) | `Sbpp\Upload\UploadHandler::handle()` (`web/includes/Upload/UploadHandler.php`). The page handler at `web/pages/admin.upload.php` is a thin wrapper passing the per-page knobs (`permission` mask, `field` `$_FILES` key, `allowed` extensions, `destDir`, `callback` JS function name on `window.opener`, `auditOk` / `auditFmt` / `errorMsg` / `title` / `formName` / `formats` strings, optional `renameToHash` for demo-style randomised filenames). The handler runs CSRF + permission check, sanitises `$_FILES[…]['name']` via `sanitiseName()` (basename + strip backslashes + trim leading dots — defends LFI on the icon / mapimage paths where the filename hits disk), `move_uploaded_file()`s to the destination, calls `Log::add(LogType::Message, …)`, and on success emits the `` blob the parent page picks up. The three reference call sites (`admin.uploaddemo.php`, `admin.uploadicon.php`, `admin.uploadmapimg.php`) are 30-line wrappers; new asset types should match that line budget. Anti-pattern: hand-rolling the move / log / popup-emission sequence per page (the pre-`goals#5` shape). | | Edit a template | `web/themes/default/*.tpl` | | Reuse the moderation-queue card layout (admin submissions / protests, mobile-stacked summary rows) | `web/themes/default/css/theme.css` (`.queue-row`, `.queue-row__body`, `.queue-row__date` — #1207 PUB-2). Apply by adding `class="queue-row …"` to the outer `
` and dropping the inline `flex` / `flex-shrink:0` styles from the summary children. | | Add visible row actions to a table-rendered admin list (Edit / Unmute / Remove buttons + responsive mobile-card mirror) | `web/themes/default/page_comms.tpl` (#1207 ADM-5) is the canonical reference: `
'; @@ -964,7 +1129,16 @@ function bansBuildComments(array $commentres, $userbank, int $rowId, string $typ if ($crow['aid'] == $userbank->GetAid() || $userbank->HasAccess(WebPermission::Owner)) { $cdata['editcomlink'] = CreateLinkR('', 'index.php?p=banlist&comment=' . $rowId . '&ctype=' . $type . '&cid=' . $crow['cid'], 'Edit Comment'); if ($userbank->HasAccess(WebPermission::Owner)) { - $cdata['delcomlink'] = ""; + // #1402: see web/scripts/comment-actions.js for the dispatcher. + // $type is the literal letter 'P' (protests) or 'S' (submissions); + // the api handler's `ctype` arm consumes both. No paginator on the + // moderation queues, so data-page is the sentinel -1. + $cdata['delcomlink'] = ''; } } else { $cdata['editcomlink'] = ""; diff --git a/web/pages/admin.edit.comms.php b/web/pages/admin.edit.comms.php index c4dbc8499..a5c4c4b29 100644 --- a/web/pages/admin.edit.comms.php +++ b/web/pages/admin.edit.comms.php @@ -36,7 +36,19 @@ isset($_GET["page"]) ? $pagelink = "&page=" . urlencode($_GET["page"]) : $pagelink = ""; -$errorScript = ""; +// #1402: per-field inline-error setters now emit vanilla DOM calls +// instead of the MooTools `$('id').setStyle('display', 'block')` shape +// that died with sourcebans.js at #1123 D1. Each error tuple becomes +// `document.getElementById(id).textContent = msg; …style.display='block'` +// in the page-tail ` +{* + #1402: comment-actions.js — single document-level click delegate + for `data-action="comment-delete"` triggers (admin moderation + queues, banlist / commslist comment editor on themes that render + delcomlink). Loaded globally because the dispatcher is feature- + detected (no-op when no triggers exist) and the four surfaces it + serves render from different page handlers; per-page includes + would mean tracking four mount points instead of one. Pre-#1402 + every trash-can click on a comment threw + `ReferenceError: RemoveComment is not defined` (the helper lived + in the deleted sourcebans.js at #1123 D1). +*} + diff --git a/web/themes/default/page_admin_admins_add.tpl b/web/themes/default/page_admin_admins_add.tpl index 9306df1d7..f86071b13 100644 --- a/web/themes/default/page_admin_admins_add.tpl +++ b/web/themes/default/page_admin_admins_add.tpl @@ -1,14 +1,31 @@ {* - SourceBans++ 2026 — admin/admins add + SourceBans++ (c) 2014-2026 SourceBans++ Dev Team + Licensed under Creative Commons Attribution-NonCommercial-ShareAlike 3.0. + See LICENSE.md for the full license text and THIRD-PARTY-NOTICES.txt for attributions. Pair: web/pages/admin.admins.php (renders this OR the list OR the overrides editor based on ?section=) and web/includes/View/AdminAdminsAddView.php. - Form submission stays on the legacy ProcessAddAdmin() helper to keep - the JSON-API contract identical to the default theme. The CSRF - protection comes from {csrf_field}; xajax/sb-callback are NOT - reintroduced. + #1402: Wires the four dead-on-v2.0 JS handlers + (`ProcessAddAdmin`, `LoadGeneratePassword`, `update_server`, + `update_web`) directly to the existing JSON API actions + (`Actions.AdminsAdd`, `Actions.AdminsGeneratePassword`) via a + page-tail vanilla-JS dispatcher. The pre-fix shape relied on + helpers from `web/scripts/sourcebans.js` (deleted at #1123 D1): + the form's `onsubmit="event.preventDefault(); if (typeof + ProcessAddAdmin === 'function') ProcessAddAdmin();"` always took + the `event.preventDefault()` path (silent no-op — the guard + swallowed the missing helper and the form never POSTed), the + "Generate password" button's onclick was the same shape, and the + ` + {* #1402: data-action="admin-add-generate-password" replaces the + dead `onclick="if (typeof LoadGeneratePassword === 'function') + LoadGeneratePassword(); return false;"` guard. The page-tail + dispatcher below calls Actions.AdminsGeneratePassword and + writes the result into #password / #password2. *} @@ -175,10 +197,13 @@
+ {* #1402: replaces the dead `onchange="if (typeof update_server === 'function') + update_server();"` guard. The page-tail dispatcher reacts to `change` + on this element via `data-action="admin-add-update-server"`. *}
-
+ {* #1402: pre-#1402 this was a hollow `
` that the + legacy `update_server()` helper was supposed to mount the new-group + name field + SourceMod flag input into. The helper was deleted with + sourcebans.js (#1123 D1); the new dispatcher reveals these inline + inputs on the right ` +
+
+ +
-
+
+ + +
@@ -222,5 +325,306 @@ {* nofilter: server-built `` from `:prefix_servers.sid` integer column, no user input — see admin.admins.php. *} {$server_script nofilter} + + {* ============================================================ + #1402 — Add-admin constructive form wiring. + + Replaces four dead JS helpers from sourcebans.js (#1123 D1): + - `ProcessAddAdmin()` → submit handler that collects fields, + builds the web-flag bitmask + server-flag string, fires + sb.api.call(Actions.AdminsAdd, …) and dispatches errors + into the per-field `.msg` slots. + - `LoadGeneratePassword()` → click handler that calls + Actions.AdminsGeneratePassword and writes the result + into #password / #password2. + - `update_server()` / `update_web()` → change handlers that + reveal the conditional inputs on "Custom permissions" / + "New admin group". + + All four were silent no-ops on v2.0 because the helpers + lived in the deleted sourcebans.js: the form's + `event.preventDefault()` swallowed every submit; the + "Generate password" button did nothing; picking "Custom + permissions" left the flag picker hidden, so an operator + who tried to ride the form ended up POSTing nothing useful + anyway. + + Constructive-form pattern mirrors `SbppGroupsAdd` in + page_admin_groups_add.tpl (canonical reference from + AGENTS.md "Add a confirm + reason modal …"). + ============================================================ *} + {literal} + + {/literal} {/if} diff --git a/web/themes/default/page_admin_bans_groups.tpl b/web/themes/default/page_admin_bans_groups.tpl index 8b103d4d4..d6fa38a1a 100644 --- a/web/themes/default/page_admin_bans_groups.tpl +++ b/web/themes/default/page_admin_bans_groups.tpl @@ -4,22 +4,32 @@ "Group ban" tab on the admin bans page. Two modes share this surface: - Default: a small form to ban a Steam community group by URL. - Submission goes through the legacy ProcessGroupBan() helper in - admin.bans.php's tail script (which dispatches to - Actions.GroupbanCheck via sb.api.call). - - "From player" mode (?fid=STEAMID): the legacy LoadGetGroups() - helper enumerates the player's group memberships into the - #steamGroupsTable list; ticking groups + clicking "Add Group Ban" - runs CheckGroupBan() to issue Actions.GroupbanCheck for each - selected group. + Submission goes through the page-tail JS in admin.bans.php + (event-delegated `data-action="groupban-*"` dispatcher → chains + `Actions.BansGroupBan` + `Actions.BansBanMemberOfGroup`). + - "From player" mode (?fid=STEAMID): the inline LoadGetGroups + helper below enumerates the player's group memberships into + the #steamGroupsTable list; ticking groups + clicking + "Add Group Ban" runs the bulk dispatch (one BansGroupBan + + BansBanMemberOfGroup pair per selected group). + + #1402 — Migrated `onclick="ProcessGroupBan();"` / + `onclick="CheckGroupBan();"` / `onclick="TickSelectAll();"` + bindings to `data-action="…"` attributes per AGENTS.md + "Add a confirm + reason modal …" (the canonical-shape rule for + rewiring dead sourcebans.js helpers). The three globals lived + in web/scripts/sourcebans.js (deleted at #1123 D1); every click + was a `ReferenceError: ProcessGroupBan is not defined` (loud + sister-shape of the #1397 / #1352 trash-can bug). The new + dispatcher in admin.bans.php's tail script binds against the + data-attributes and uses sb.api.call → window.SBPP.setBusy / + showToast for the loading state + final feedback. - Both helpers (LoadGetGroups, TickSelectAll, CheckGroupBan) live in - web/scripts/sourcebans.js and aren't loaded by the sbpp2026 chrome; - that flow remains a default-theme feature for the rollout window. DOM ids (groupurl, groupreason, *.msg, agban, aback, gban, tickswitch, tickswitchlink, steamGroups, steamGroupsText, - steamGroupsTable, steamGroupStatus) are preserved so legacy callers - continue to find them on default. + steamGroupsTable, steamGroupStatus) are preserved so existing + LoadGetGroups inline script + any third-party theme that wired + extra behaviour on top still finds them. `$player_name` is rendered above the group list when reaching this tab from a banlist row (?fid=STEAMID&player=…). admin.bans.php @@ -59,7 +69,8 @@ + data-action="groupban-submit-form" + onsubmit="event.preventDefault(); return false;"> {csrf_field}
@@ -88,11 +99,16 @@ id="aback" data-testid="groupban-back" onclick="history.go(-1);">Back + {* #1402: was `onclick="ProcessGroupBan();"` which threw + ReferenceError post-#1123 D1 (the helper lived in the + deleted sourcebans.js). The page-tail dispatcher in + admin.bans.php picks up data-action="groupban-submit" + and chains Actions.BansGroupBan → BansBanMemberOfGroup. *}
@@ -122,20 +138,22 @@ + {* #1402: was `onclick="TickSelectAll();"` (dead since #1123 D1). *} Group + {* #1402: was `onclick="TickSelectAll();return false;"`. *} Select all @@ -151,12 +169,16 @@
+ {* #1402: was `onclick="CheckGroupBan();"` which threw + ReferenceError. The page-tail dispatcher in + admin.bans.php picks up data-action="groupban-bulk-submit" + and iterates ticked rows through the same chain. *}
diff --git a/web/themes/default/page_admin_edit_mod.tpl b/web/themes/default/page_admin_edit_mod.tpl index 78d8057eb..639c1b67a 100644 --- a/web/themes/default/page_admin_edit_mod.tpl +++ b/web/themes/default/page_admin_edit_mod.tpl @@ -16,6 +16,14 @@ emit a CSP-friendly inline script that calls `window.opener.icon()` via the modernised UploadHandler chrome. + #1402: the page-tail script below wires `window.opener.icon` so + the popup's success callback (UploadHandler.php line 187, calling + `window.opener.icon()`) actually has somewhere + to land — pre-fix the parent window had no `icon` function + defined, so the upload popup threw `TypeError: window.opener.icon + is not a function`, stayed open, and the chosen icon never + reached the form's hidden `#icon_hid` input. + Initial checkbox state is server-rendered via the new `$enabled` template variable — no MooTools-era `$('enabled').checked = …` re-paint script. @@ -139,4 +147,36 @@ + +{* #1402: wire `window.opener.icon` so admin.uploadicon.php's success + blob (emitted by Sbpp\Upload\UploadHandler::handle with + `callback: 'icon'`) can write the filename back into this form. The + pre-fix shape relied on a `window.icon` definition that lived in the + pre-v2.0.0 sourcebans.js bulk file (#1123 D1 deleted it), so the + popup's `window.opener.icon()` call threw `TypeError: + window.opener.icon is not a function`, the popup stayed open, and + the chosen icon never propagated. The handler also patches the + visible "Current: …" preview chip so the operator sees what's been + picked. *} +{literal} + +{/literal} diff --git a/web/themes/default/page_admin_mods_add.tpl b/web/themes/default/page_admin_mods_add.tpl index 37c671af2..19b856f58 100644 --- a/web/themes/default/page_admin_mods_add.tpl +++ b/web/themes/default/page_admin_mods_add.tpl @@ -1,23 +1,37 @@ {* - SourceBans++ 2026 — page / page_admin_mods_add.tpl + SourceBans++ (c) 2014-2026 SourceBans++ Dev Team + Licensed under Creative Commons Attribution-NonCommercial-ShareAlike 3.0. + See LICENSE.md for the full license text and THIRD-PARTY-NOTICES.txt for attributions. Second tab of the admin "Mods" page (add a new mod). Pair: Sbpp\View\AdminModsAddView + web/pages/admin.mods.php. - Submission flow (preserved end-to-end from the legacy theme): - 1. +
+ + +

16x16 GIF, PNG or JPG. Opens a popup uploader.

@@ -143,5 +166,166 @@ + + {* ============================================================ + #1402 — Add-mod constructive form wiring (inline page-tail JS). + + Replaces the v1.x `ProcessMod()` helper (deleted with + sourcebans.js at #1123 D1) — pre-fix the form's + `onsubmit="ProcessMod(); return false;"` swallowed the native + submit but never dispatched to anything, so the Add-mod button + was a silent no-op (no console error, no toast, no API call, + no row created). Also wires `window.opener.icon()` (called by + admin.uploadicon.php's success blob via + Sbpp\Upload\UploadHandler::handle's `callback: 'icon'`) into + the hidden `#icon_hid` input so the chosen icon filename + actually rides the form submission. + + Constructive-form pattern mirrors `SbppGroupsAdd` in + page_admin_groups_add.tpl: intercept submit, client-side + validate, busy-flip the submit button via SBPP.setBusy, fire + sb.api.call(Actions.ModsAdd, …), branch on the envelope. + + No `// @ts-check` here because the file is rendered by Smarty; + ts-check only runs against `.js` sources in `web/scripts`. + ============================================================ *} + {literal} + + {/literal} {/if} diff --git a/web/themes/default/page_bans.tpl b/web/themes/default/page_bans.tpl index 2f4eff3ff..9e7aaf594 100644 --- a/web/themes/default/page_bans.tpl +++ b/web/themes/default/page_bans.tpl @@ -891,6 +891,11 @@ the listing branch silently broke comment save (no submit handler attached, native form submission to action-less URL no-ops). *} +{* #1402: trash-can-on-a-comment triggers (`data-action="comment-delete"`) + on the comment-edit branch are handled by the global comment-actions.js + dispatcher loaded from core/footer.tpl — single mount point shared + with the admin moderation queues (protests / submissions) and the + commslist comment-edit branch. *} {* ============================================================ #1301 — banlist row-action wiring (inline page-tail JS). From b7e78c912d0794413b305ed9fe006c75df7f4e3a Mon Sep 17 00:00:00 2001 From: rumblefrog Date: Sun, 17 May 2026 19:10:05 -0400 Subject: [PATCH 2/2] fix(admin): address adversarial review findings on #1402 rewire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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
— 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 --- web/api/handlers/admins.php | 23 +++ web/includes/View/AdminAdminsAddView.php | 7 + web/pages/admin.admins.php | 5 + web/pages/admin.bans.php | 19 ++ web/scripts/comment-actions.js | 12 ++ web/tests/api/AdminsTest.php | 57 ++++++ .../e2e/specs/flows/admins-add-form.spec.ts | 186 +++++++++++++++++- .../flows/comment-delete-dispatcher.spec.ts | 11 +- .../specs/flows/groupban-dispatcher.spec.ts | 7 +- .../e2e/specs/flows/mods-add-form.spec.ts | 9 +- web/themes/default/core/footer.tpl | 6 +- web/themes/default/page_admin_admins_add.tpl | 182 +++++++++++++++-- web/themes/default/page_admin_bans_groups.tpl | 1 - web/themes/default/page_admin_mods_add.tpl | 10 + 14 files changed, 505 insertions(+), 30 deletions(-) diff --git a/web/api/handlers/admins.php b/web/api/handlers/admins.php index b89197b27..fb6fbe2b3 100644 --- a/web/api/handlers/admins.php +++ b/web/api/handlers/admins.php @@ -134,6 +134,29 @@ function api_admins_add(array $params): array if ($serverName === '0' || $serverName === '') $serverName = null; + // #1402 (adversarial review HIGH 1) — OWNER-flag privilege-escalation + // guard. Mirrors api_admins_edit_perms's check (see below in this + // file). Two ways an attacker could land `ADMIN_OWNER` on disk via + // this handler: + // 1. `web_group === 'c'` (Custom permissions) + `mask & ADMIN_OWNER` + // → goes straight onto `:prefix_admins.extraflags`. + // 2. `web_group === 'n'` (New admin group) + `mask & ADMIN_OWNER` + // → baked into the new `:prefix_groups.flags`, after which any + // future admin assigned to that group inherits it. + // Both shapes carry the OWNER bit in the inbound `mask` param, so a + // single check on `$mask & ADMIN_OWNER` covers both. The bare check + // does NOT close the existing-group escalation surface (assigning a + // pre-existing OWNER-bearing group to a new admin via `web_group` = + // an integer > 0); that path was pre-existing pre-#1402 and is + // tracked separately. The UI side mirrors this by gating the OWNER + // checkbox itself on `can_grant_owner` so non-owners don't see the + // affordance — defense in depth. + if (!$userbank->HasAccess(WebPermission::Owner) && ($mask & ADMIN_OWNER)) { + Log::add(LogType::Warning, 'Hacking Attempt', + $userbank->GetProperty('user') . ' tried to grant OWNER while adding an admin, but doesnt have access.'); + return Api::redirect('index.php?p=login&m=no_access'); + } + // Validation ------------------------------------------------------- if (empty($name)) { throw new ApiError('validation', 'You must type a name for the admin.', 'name'); diff --git a/web/includes/View/AdminAdminsAddView.php b/web/includes/View/AdminAdminsAddView.php index 606f100bf..ea6204534 100644 --- a/web/includes/View/AdminAdminsAddView.php +++ b/web/includes/View/AdminAdminsAddView.php @@ -25,6 +25,12 @@ final class AdminAdminsAddView extends View * rows for the SourceMod admin-group dropdown. * @param list> $server_group_list `:prefix_groups` * rows (type != 3) for the web admin-group dropdown. + * @param bool $can_grant_owner Mirrors `$userbank->HasAccess(WebPermission::Owner)` + * — gates the OWNER web-flag checkbox so a non-owner with + * `ADMIN_ADD_ADMINS` can't see/tick it (the server-side check in + * `api_admins_add` is the load-bearing gate; this is the + * visible-affordance half of the defense-in-depth pair from + * #1402's adversarial review). */ public function __construct( public readonly bool $can_add_admins, @@ -33,6 +39,7 @@ public function __construct( public readonly array $server_admin_group_list, public readonly array $server_group_list, public readonly string $server_script, + public readonly bool $can_grant_owner, ) { } } diff --git a/web/pages/admin.admins.php b/web/pages/admin.admins.php index 6c1f4fb58..826f31e60 100644 --- a/web/pages/admin.admins.php +++ b/web/pages/admin.admins.php @@ -147,6 +147,11 @@ server_admin_group_list: $server_admin_group_list, server_group_list: $server_group_list, server_script: $serverscript, + // #1402 adversarial review HIGH 1: gate the OWNER checkbox so a + // non-owner with ADMIN_ADD_ADMINS can't see/tick it. Paired with + // the server-side guard in api_admins_add (the load-bearing + // half; this is the visible-affordance half). + can_grant_owner: $userbank->HasAccess(WebPermission::Owner), )); echo ''; return; diff --git a/web/pages/admin.bans.php b/web/pages/admin.bans.php index 313faa1e2..224bce3ef 100644 --- a/web/pages/admin.bans.php +++ b/web/pages/admin.bans.php @@ -973,6 +973,13 @@ function status(html) { function loadGroupBan(groupuri, isgrpurl, queue, reason, last, submitBtn) { var a = api(), A = actions(); if (!a || !A) return Promise.resolve(); + // #1402 adversarial review MEDIUM 4: defensive .catch() arms on + // BOTH the outer Actions.BansGroupBan and inner + // Actions.BansBanMemberOfGroup chains so a throw inside either + // success callback (or a sb.api.call internal failure) doesn't + // leave the submit button busy forever. The legacy single-URL + // path and the bulk-from-friends loop both share this helper, + // so a flaky branch in either path is silently captured here. return a.call(A.BansGroupBan, { groupuri: groupuri, isgrpurl: isgrpurl, @@ -1023,7 +1030,19 @@ function loadGroupBan(groupuri, isgrpurl, queue, reason, last, submitBtn) { toast('success', 'Group banned', body, 'index.php?p=banlist'); status('' + body + ''); } + }).catch(function (err2) { + // Inner-call defensive: release the button + surface + // the error so a bulk loop doesn't silently stall + // after one row throws. + setBusy(submitBtn, false); + toast('error', 'Group ban failed', String(err2 && err2.message ? err2.message : err2)); + status(''); }); + }).catch(function (err) { + // Outer-call defensive: same shape as the inner catch. + setBusy(submitBtn, false); + toast('error', 'Group ban failed', String(err && err.message ? err.message : err)); + status(''); }); } diff --git a/web/scripts/comment-actions.js b/web/scripts/comment-actions.js index 378cbee18..88bae0d06 100644 --- a/web/scripts/comment-actions.js +++ b/web/scripts/comment-actions.js @@ -126,6 +126,18 @@ if (msg.redir) window.location.href = msg.redir; else window.location.reload(); }, 1200); + }).catch(function (err) { + // #1402 adversarial review MEDIUM 4: defensive .catch() arm + // so a throw inside the success callback (or a sb.api.call + // internal failure) doesn't leave the trash-can stuck in + // its busy state. The trash-can appears in dense threads + // (potentially 10+ per page) and a stuck row reads as a + // broken affordance — the operator clicks again, gets the + // confirm prompt, and the second click stays no-op'd + // because the bubble-phase delegate sees `aria-busy` and + // the dispatch silently re-fires. + setBusy(trigger, false); + toast('error', 'Delete failed', String(err && err.message ? err.message : err)); }); }); })(); diff --git a/web/tests/api/AdminsTest.php b/web/tests/api/AdminsTest.php index 1d1e64c49..3427151a8 100644 --- a/web/tests/api/AdminsTest.php +++ b/web/tests/api/AdminsTest.php @@ -295,6 +295,63 @@ public function testEditPermsBlocksGrantingOwnerWithoutOwner(): void $this->assertSame('index.php?p=login&m=no_access', $env['redirect'] ?? null); } + /** + * #1402 adversarial review HIGH 1 — `api_admins_add` must reject + * `mask & ADMIN_OWNER` when the caller doesn't hold OWNER. Mirrors + * the existing `testEditPermsBlocksGrantingOwnerWithoutOwner` shape + * because the two handlers share the same escalation surface + * (set web flags on an admin row). + * + * Pre-fix `api_admins_add` had no such check — a non-owner with + * `ADMIN_ADD_ADMINS` (a common delegation level) could create a + * brand-new admin with the OWNER bit set on their `extraflags`, + * full panel takeover. The UI side gates the OWNER checkbox on + * `can_grant_owner`; the server-side check below is the + * load-bearing half (UI gate is defense-in-depth). + */ + public function testAddBlocksGrantingOwnerWithoutOwner(): void + { + // Seed a non-owner admin with ADMIN_ADD_ADMINS (the registry- + // declared perm for `admins.add`) and log in as them. + $pdo = Fixture::rawPdo(); + $hash = password_hash('admin', PASSWORD_BCRYPT); + $pdo->prepare(sprintf( + 'INSERT INTO `%s_admins` (user, authid, password, gid, email, validate, extraflags, immunity) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)', + DB_PREFIX + ))->execute(['nonowner-add', 'STEAM_0:0:111', $hash, -1, 'noown-add@own.test', null, ADMIN_ADD_ADMINS, 0]); + $nonOwnerAid = (int)$pdo->lastInsertId(); + + $this->loginAs($nonOwnerAid); + $env = $this->api('admins.add', $this->adminParams([ + 'name' => 'TargetOwner', + 'steam' => 'STEAM_0:0:9001', + 'mask' => ADMIN_OWNER, + ])); + // The handler returns Api::redirect on the OWNER escalation attempt. + $this->assertFalse($env['ok'] ?? true); + $this->assertSame('index.php?p=login&m=no_access', $env['redirect'] ?? null); + // And no row landed on disk. + $this->assertNull($this->row('admins', ['authid' => 'STEAM_0:0:9001'])); + } + + /** + * Sibling positive case: an owner can still create OWNER admins. + * Locks in that the new guard isn't over-zealous on the happy path + * (an owner-only operation that legitimately needs the bit). + */ + public function testAddAllowsGrantingOwnerForOwner(): void + { + $this->loginAsAdmin(); // The seeded admin holds ADMIN_OWNER. + $env = $this->api('admins.add', $this->adminParams([ + 'name' => 'AnotherOwner', + 'steam' => 'STEAM_0:0:9002', + 'mask' => ADMIN_OWNER, + ])); + $this->assertTrue($env['ok'], json_encode($env)); + $this->assertNotNull($this->row('admins', ['authid' => 'STEAM_0:0:9002'])); + } + public function testGeneratePasswordReturnsString(): void { $this->loginAsAdmin(); diff --git a/web/tests/e2e/specs/flows/admins-add-form.spec.ts b/web/tests/e2e/specs/flows/admins-add-form.spec.ts index 361232637..af7aa8a9c 100644 --- a/web/tests/e2e/specs/flows/admins-add-form.spec.ts +++ b/web/tests/e2e/specs/flows/admins-add-form.spec.ts @@ -152,10 +152,15 @@ test.describe('flow: admin admins add form (#1402 — ProcessAddAdmin zombie)', const pw2 = await page.locator('[data-testid="admin-add-password2"]').inputValue(); expect(pw1).toBe(env.data.password); expect(pw2).toBe(env.data.password); - // The page-tail script flips the type from "password" to - // "text" so the operator can read what was generated. + // #1402 adversarial review MEDIUM 5: the input types must + // stay as `password` — the legacy `LoadGeneratePassword` + // helper never flipped `.type`, and leaving the generated + // value visible indefinitely is a privacy / shoulder-surf / + // screenshot leak. expect(await page.locator('[data-testid="admin-add-password"]').getAttribute('type')) - .toBe('text'); + .toBe('password'); + expect(await page.locator('[data-testid="admin-add-password2"]').getAttribute('type')) + .toBe('password'); }); test('Server-group "New admin group" reveals new-name + SM flags inputs', async ({ page }) => { @@ -200,4 +205,179 @@ test.describe('flow: admin admins add form (#1402 — ProcessAddAdmin zombie)', await expect(webNewName).toBeVisible(); await expect(ownerFlagCb).toBeVisible(); }); + + /** + * #1402 adversarial review HIGH 3 (stale-flags ride-through). + * + * Pre-fix `updateServer` / `updateWeb` only toggled the `hidden` + * attribute on the dependent blocks — the checkbox values + text + * inputs survived a dropdown flip. `collectWebFlags()` walked + * the unscoped `#web-flags-block input[data-flag]` and the + * submit handler read `#server-flags` / `#*-new-name` + * unconditionally. The repro: + * 1. Select "Custom permissions" → reveals the flag picker. + * 2. Tick "Owner" (or any other ADMIN_* checkbox). + * 3. Flip dropdown back to "No permissions" → block hides. + * 4. Submit → API call ships `mask: ADMIN_OWNER` despite the + * final UI saying "no permissions". + * Two routes to accidental OWNER grant (the other being HIGH 1's + * uncondionally-rendered checkbox). + * + * Post-fix the helpers clear the dependent inputs AND the + * collectors are scoped to `:not([hidden])` so even if the + * clear ever stops firing, a hidden checkbox can't ride into + * the mask. + */ + test('Dropdown flip → "Custom permissions" → tick OWNER → "No permissions" → submit ships mask: 0', async ({ page }) => { + // Intercept the AdminsAdd request so we can inspect the + // serialised mask without needing to stub it (we still want + // to hit the real handler to assert end-to-end). + let lastMask: number | null = null; + await page.route('**/api.php', async (route) => { + try { + const body = JSON.parse(route.request().postData() || '{}'); + if (body?.action === 'admins.add') { + lastMask = Number(body?.params?.mask ?? -1); + } + } catch { + /* swallow JSON parse errors on non-admins.add calls */ + } + await route.continue(); + }); + + await page.goto(ADMIN_ADMINS_ADD_ROUTE); + + await page.locator('[data-testid="admin-add-name"]').fill('stale-flag-victim'); + await page.locator('[data-testid="admin-add-steam"]').fill('STEAM_0:0:88008800'); + await page.locator('[data-testid="admin-add-email"]').fill('stale@flag.test'); + await page.locator('[data-testid="admin-add-password"]').fill('somepassword'); + await page.locator('[data-testid="admin-add-password2"]').fill('somepassword'); + await page.locator('[data-testid="admin-add-serverg"]').selectOption('-3'); + + // Walk the trap: reveal flag picker, tick OWNER, hide flag + // picker. The post-fix updateWeb() clears the checkbox AND + // the collector skips hidden ancestors — both pin the mask + // at 0 regardless of which guard fires first. + await page.locator('[data-testid="admin-add-webg"]').selectOption('c'); + const ownerCb = page.locator('[data-testid="admin-add-flag-owner"]'); + await expect(ownerCb).toBeVisible(); + await ownerCb.check(); + // Now flip back to "No permissions" — the checkbox should be + // cleared AND the block re-hidden. + await page.locator('[data-testid="admin-add-webg"]').selectOption('-3'); + await expect(ownerCb).toBeHidden(); + + const responsePromise = page.waitForResponse( + (r) => + r.url().includes('api.php') && + r.request().method() === 'POST' && + r.status() === 200, + ); + await page.locator('[data-testid="admin-add-submit"]').click(); + const env = await (await responsePromise).json(); + + // The mask shipped to the API must be 0 — neither path + // (clearWebFlags clears the checkbox, collectWebFlags skips + // hidden ancestors) should let the OWNER bit slip through. + expect(lastMask, 'submit must NOT smuggle stale OWNER bit').toBe(0); + // The handler validation succeeded so the new admin landed + // with no extra flags. + expect(env.ok, JSON.stringify(env)).toBe(true); + }); + + /** + * #1402 adversarial review HIGH 2 (rehash silently dropped). + * + * The legacy ProcessAddAdmin consumed `data.rehash` from + * api_admins_add's envelope and fired `Actions.SystemRehashAdmins` + * so the SourceMod plugins on the relevant game servers reloaded + * their admin lists. The rewrite's first cut read `data.message` + * only and navigated away. config.enableadminrehashing defaults + * to '1' in data.sql, so the rehash is the expected default — + * without it, a brand-new admin can log in to the panel but + * can't moderate on game servers until the next server restart. + * + * Test shape: stub both API calls so we can verify the chain + * without needing real `:prefix_servers` rows that the new + * admin has access to (the seeded DB's admin holds no per- + * server group memberships, so the natural `rehash` from a + * real call is null). + */ + test('Success path → chains Actions.SystemRehashAdmins when handler returns rehash sids', async ({ page }) => { + /** @type {{action?:string,params?:Record}[]} */ + const apiCalls: { action?: string; params?: Record }[] = []; + await page.route('**/api.php', async (route) => { + let body: { action?: string; params?: Record } | null = null; + try { + body = JSON.parse(route.request().postData() || '{}'); + } catch { + body = null; + } + if (!body || !body.action) { + await route.continue(); + return; + } + apiCalls.push(body); + if (body.action === 'admins.add') { + // Synthesise a rehash payload — two server ids. The + // wire envelope matches `Api::dispatch`'s shape: + // `{ok: true, data: }`. Without this + // wrapping the dispatcher's success branch reads + // `r.data.rehash` as `undefined` and the rehash chain + // never fires. + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + ok: true, + data: { + aid: 4242, + reload: true, + rehash: '1,2', + message: { + title: 'Admin Added', + body: 'The admin has been added successfully', + kind: 'green', + redir: 'index.php?p=admin&c=admins', + }, + }, + }), + }); + return; + } + if (body.action === 'system.rehash_admins') { + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ ok: true, data: { rehashed: 2 } }), + }); + return; + } + await route.continue(); + }); + + await page.goto(ADMIN_ADMINS_ADD_ROUTE); + + await page.locator('[data-testid="admin-add-name"]').fill('rehash-target'); + await page.locator('[data-testid="admin-add-steam"]').fill('STEAM_0:0:7777'); + await page.locator('[data-testid="admin-add-email"]').fill('rehash@target.test'); + await page.locator('[data-testid="admin-add-password"]').fill('somepassword'); + await page.locator('[data-testid="admin-add-password2"]').fill('somepassword'); + await page.locator('[data-testid="admin-add-serverg"]').selectOption('-3'); + await page.locator('[data-testid="admin-add-webg"]').selectOption('-3'); + + await page.locator('[data-testid="admin-add-submit"]').click(); + + // Wait for the chained call. The dispatcher fires + // AdminsAdd → SystemRehashAdmins → navigate; the rehash + // arm must land before the 1200ms navigation timeout. + await expect.poll(() => apiCalls + .map((c) => c.action) + .filter((a) => a === 'admins.add' || a === 'system.rehash_admins'), + ).toEqual(['admins.add', 'system.rehash_admins']); + + const rehashCall = apiCalls.find((c) => c.action === 'system.rehash_admins'); + expect(rehashCall?.params?.servers, 'rehash call must carry the sids the handler emitted') + .toBe('1,2'); + }); }); diff --git a/web/tests/e2e/specs/flows/comment-delete-dispatcher.spec.ts b/web/tests/e2e/specs/flows/comment-delete-dispatcher.spec.ts index f59b919e6..c2f71fde1 100644 --- a/web/tests/e2e/specs/flows/comment-delete-dispatcher.spec.ts +++ b/web/tests/e2e/specs/flows/comment-delete-dispatcher.spec.ts @@ -170,10 +170,13 @@ test.describe('flow: comment-delete dispatcher (#1402 — RemoveComment zombie)' await page.locator('[data-testid="synth-delcomlink-cancel"]').click(); - // Give the dispatcher a moment to fire (or not). The - // dismiss path runs synchronously after the confirm - // resolves, so 300ms is plenty. - await page.waitForTimeout(300); + // The cancelled-confirm path returns synchronously from the + // dispatcher (window.confirm → false → return without + // touching the API). Playwright's click() awaits the click + // event's handlers, so by the time the awaited click resolves + // the dispatcher has already early-returned. No settle timer + // needed (AGENTS.md "Playwright E2E specifics" flags + // `waitForTimeout` for negative assertions as an anti-pattern). expect(apiCalls, 'cancelled confirm must NOT call the API').toBe(0); }); diff --git a/web/tests/e2e/specs/flows/groupban-dispatcher.spec.ts b/web/tests/e2e/specs/flows/groupban-dispatcher.spec.ts index 4ea98915c..8d9d48838 100644 --- a/web/tests/e2e/specs/flows/groupban-dispatcher.spec.ts +++ b/web/tests/e2e/specs/flows/groupban-dispatcher.spec.ts @@ -98,8 +98,11 @@ test.describe('flow: admin bans group-ban dispatcher (#1402 — LoadGroupBan zom await expect(errSlot).toBeVisible(); await expect(errSlot).toContainText(/group link/i); - // Brief settle window so a stray API call would surface. - await page.waitForTimeout(200); + // The inline error renders synchronously inside the click + // handler — once `errSlot` is visible we know the dispatcher + // already short-circuited. No need for a settle timer (which + // is the canonical Playwright anti-pattern flagged by + // AGENTS.md "Playwright E2E specifics"). expect(apiCalls, 'empty URL submit must NOT call the API').toBe(0); expect( diff --git a/web/tests/e2e/specs/flows/mods-add-form.spec.ts b/web/tests/e2e/specs/flows/mods-add-form.spec.ts index 6220a5a4c..5fd80fc3f 100644 --- a/web/tests/e2e/specs/flows/mods-add-form.spec.ts +++ b/web/tests/e2e/specs/flows/mods-add-form.spec.ts @@ -130,12 +130,15 @@ test.describe('flow: admin mods add form (#1402 — ProcessMod zombie)', () => { await page.locator('[data-testid="addmod-submit"]').click(); // Inline error slot lights up (the page-tail script writes - // into the `.msg` div next to the icon field). + // into the `.msg` div next to the icon field). Once the + // error is visible, the JS gate has already returned and + // the dispatcher never reached the API — no settle timer + // needed (AGENTS.md "Playwright E2E specifics" flags + // `waitForTimeout` for negative assertions as an + // anti-pattern). const iconError = page.locator('#icon\\.msg'); await expect(iconError).toContainText(/icon/i); - // Brief settle window so a stray API call would surface. - await page.waitForTimeout(200); expect(apiCalls, 'invalid form must NOT POST to the API').toBe(0); }); }); diff --git a/web/themes/default/core/footer.tpl b/web/themes/default/core/footer.tpl index 4c055cd2d..40df90446 100644 --- a/web/themes/default/core/footer.tpl +++ b/web/themes/default/core/footer.tpl @@ -185,7 +185,11 @@ `ReferenceError: RemoveComment is not defined` (the helper lived in the deleted sourcebans.js at #1123 D1). *} - +{* `defer` would be a no-op here since the script lives at the body + tail and the parser is already past the body. Drop it so the + markup matches the runtime behaviour (#1402 adversarial review + LOW 8). *} + diff --git a/web/themes/default/page_admin_admins_add.tpl b/web/themes/default/page_admin_admins_add.tpl index f86071b13..cf06403c7 100644 --- a/web/themes/default/page_admin_admins_add.tpl +++ b/web/themes/default/page_admin_admins_add.tpl @@ -276,7 +276,15 @@ data-flag values map to ADMIN_* names so the JS dispatcher can OR them with Perms.ADMIN_*. *} - + {if $can_grant_owner} + {* #1402 adversarial review HIGH 1: OWNER is gated. + A non-owner with ADMIN_ADD_ADMINS otherwise sees + the checkbox and can grant OWNER (full panel + takeover). Server-side guard in api_admins_add + is the load-bearing pair; this is the visible- + affordance half. *} + + {/if} @@ -401,9 +409,43 @@ }); } + /** + * Clear every web-flag checkbox inside #web-flags-block and + * the SourceMod flags / new-group name text inputs. Called + * by updateServer() / updateWeb() when the dropdown swings + * back to a value that hides the dependent block. Pre-fix + * (#1402 adversarial review HIGH 3), the helpers only + * toggled the `hidden` attribute and left the inputs' + * values intact, so an operator who ticked Owner under + * "Custom permissions" and then flipped the dropdown to + * "No permissions" still submitted `mask = ADMIN_OWNER` + * (silent OWNER grant). Mirrored on both the web + server + * sides. + */ + function clearWebFlags() { + document.querySelectorAll('#web-flags-block input[data-flag]').forEach(function (cb) { + /** @type {HTMLInputElement} */ (cb).checked = false; + }); + } + function clearServerFlags() { + var srvFlags = /** @type {HTMLInputElement|null} */ (document.getElementById('server-flags')); + if (srvFlags) srvFlags.value = ''; + } + function clearWebNewName() { + var el = /** @type {HTMLInputElement|null} */ (document.getElementById('web-new-name')); + if (el) el.value = ''; + } + function clearServerNewName() { + var el = /** @type {HTMLInputElement|null} */ (document.getElementById('server-new-name')); + if (el) el.value = ''; + } + /** * update_server() replacement: react to #serverg change and - * reveal the matching conditional inputs. + * reveal the matching conditional inputs. Clears the + * dependent blocks' values when the dropdown swings back + * to a value that hides them (#1402 adversarial review + * HIGH 3 — stale-flags ride-through fix). */ function updateServer() { var sel = /** @type {HTMLSelectElement|null} */ (document.getElementById('serverg')); @@ -417,14 +459,21 @@ } else if (v === 'c') { nameBlock.setAttribute('hidden', ''); flagsBlock.removeAttribute('hidden'); + clearServerNewName(); } else { nameBlock.setAttribute('hidden', ''); flagsBlock.setAttribute('hidden', ''); + clearServerNewName(); + clearServerFlags(); } } /** * update_web() replacement: react to #webg change and reveal - * the matching conditional inputs. + * the matching conditional inputs. Clears the dependent + * blocks' values when the dropdown swings back to a value + * that hides them (#1402 adversarial review HIGH 3 — + * stale-flags ride-through fix; particularly important + * because the OWNER bit lives here). */ function updateWeb() { var sel = /** @type {HTMLSelectElement|null} */ (document.getElementById('webg')); @@ -438,9 +487,12 @@ } else if (v === 'c') { nameBlock.setAttribute('hidden', ''); flagsBlock.removeAttribute('hidden'); + clearWebNewName(); } else { nameBlock.setAttribute('hidden', ''); flagsBlock.setAttribute('hidden', ''); + clearWebNewName(); + clearWebFlags(); } } @@ -450,13 +502,20 @@ * integer mask. We use `+=` rather than `|=` to keep the * 32-bit-unsigned high bits intact (JS bitwise ops promote * to signed int32, which drops bits above 2^31). + * + * Defensively scopes to `#web-flags-block:not([hidden])` + * so a checkbox the operator ticked under "Custom + * permissions" and then re-hid by flipping the dropdown + * back to "No permissions" can't ride into the mask even + * if `clearWebFlags()` ever stops firing (#1402 adversarial + * review HIGH 3 — belt + suspenders on top of the clear). * @returns {number} */ function collectWebFlags() { var P = perms(); if (!P) return 0; var mask = 0; - document.querySelectorAll('#web-flags-block input[data-flag]').forEach(function (cb) { + document.querySelectorAll('#web-flags-block:not([hidden]) input[data-flag]').forEach(function (cb) { var el = /** @type {HTMLInputElement} */ (cb); if (!el.checked) return; var flagName = el.getAttribute('data-flag') || ''; @@ -467,6 +526,36 @@ }); return mask; } + /** + * Mirror of `collectWebFlags`'s defensive hidden-scoping + * for the SourceMod flags string. Returns '' when the + * server flags block is hidden so a stale value can't + * ride into `srv_mask` after a dropdown flip. + * @returns {string} + */ + function collectServerFlags() { + var flagsBlock = document.getElementById('server-flags-block'); + if (!flagsBlock || flagsBlock.hasAttribute('hidden')) return ''; + var el = /** @type {HTMLInputElement|null} */ (document.getElementById('server-flags')); + return el ? el.value.trim() : ''; + } + /** + * Mirror of `collectWebFlags`'s defensive hidden-scoping + * for the new-group name inputs. + * @returns {string} + */ + function collectWebNewName() { + var block = document.getElementById('web-new-name-block'); + if (!block || block.hasAttribute('hidden')) return ''; + var el = /** @type {HTMLInputElement|null} */ (document.getElementById('web-new-name')); + return el ? el.value.trim() : ''; + } + function collectServerNewName() { + var block = document.getElementById('server-new-name-block'); + if (!block || block.hasAttribute('hidden')) return ''; + var el = /** @type {HTMLInputElement|null} */ (document.getElementById('server-new-name')); + return el ? el.value.trim() : ''; + } /** * Collect comma-separated `g` server-group selections. @@ -515,10 +604,30 @@ var p2 = /** @type {HTMLInputElement|null} */ (document.getElementById('password2')); if (p1) p1.value = String(r.data.password); if (p2) p2.value = String(r.data.password); - // Flip both fields to type=text briefly so the operator - // can see what was generated (matches v1.x UX). - if (p1) p1.type = 'text'; - if (p2) p2.type = 'text'; + // #1402 adversarial review MEDIUM 5: leave the input + // types as `password` (matches v1.x `LoadGeneratePassword` + // — the legacy helper never flipped .type either). + // The pre-fix `type='text'` change was a privacy / + // shoulder-surf / screenshot leak: the freshly- + // generated password sat in plaintext on the operator's + // screen indefinitely after the click, even after the + // operator left the field. Operators who genuinely + // need to see the value can copy it into their + // password manager from the password field's clipboard + // (browsers + extensions both support this) or use + // their browser's "show password" toggle on a per- + // field basis. + }).catch(function (err) { + // sb.api.call only rejects on internal failures (it + // catches fetch / json errors and synthesises an + // error envelope), but defensive .catch() ensures the + // button doesn't stay busy if a throw escapes the + // success callback (e.g., DOM nodes vanished mid- + // request). Per the AGENTS.md "Loading state on + // action buttons" rule, setBusy(btn, false) must + // fire on every non-navigating response branch. + setBusy(btn, false); + toast('error', 'Generate password failed', String(err && err.message ? err.message : err)); }); }); @@ -551,9 +660,16 @@ var serverPassword = useSrvPass ? srvPassEl.value : '-1'; var sg = (/** @type {HTMLSelectElement} */ (document.getElementById('serverg'))).value; var wg = (/** @type {HTMLSelectElement} */ (document.getElementById('webg'))).value; - var serverNewName = (/** @type {HTMLInputElement} */ (document.getElementById('server-new-name'))).value.trim(); - var webNewName = (/** @type {HTMLInputElement} */ (document.getElementById('web-new-name'))).value.trim(); - var srvFlags = (/** @type {HTMLInputElement} */ (document.getElementById('server-flags'))).value.trim(); + // #1402 adversarial review HIGH 3: collect via the + // hidden-scoped helpers so a value left over from a + // previously-revealed block can't ride into the API + // call after the dropdown swung to "No permissions". + // Both updateServer/updateWeb clear the inputs AND + // these collectors fall through to '' when the parent + // block is hidden — belt + suspenders. + var serverNewName = collectServerNewName(); + var webNewName = collectWebNewName(); + var srvFlags = collectServerFlags(); var webMask = collectWebFlags(); var submitBtn = /** @type {HTMLButtonElement|null} */ (form.querySelector('[data-testid="admin-add-submit"]')); @@ -611,11 +727,45 @@ // was unreachable from a dead submit handler). var green = document.getElementById('msg-green'); if (green) green.style.display = ''; - // Leave the button busy across the navigation so the form - // can't be re-submitted while the redirect resolves. - setTimeout(function () { - window.location.href = (msg.redir || 'index.php?p=admin&c=admins'); - }, 1200); + + // #1402 adversarial review HIGH 2: chain + // `Actions.SystemRehashAdmins` when the handler tells + // us the new admin's per-server access requires a + // rehash. The legacy ProcessAddAdmin path consumed + // `data.rehash`; the rewrite silently dropped it, + // which meant new admins could log in to the panel + // but couldn't moderate on game servers until the + // next server restart (config.enableadminrehashing + // defaults to '1' in data.sql — the rehash is the + // expected default behaviour). Mirrors the + // _admin_edit_helpers.php:fireRehash shape (same + // catch arm so a flaky rehash endpoint still resolves + // the navigation). + var rehashSids = (data.rehash || '').toString(); + var navigate = function () { + // Leave the button busy across the navigation so + // the form can't be re-submitted while the redirect + // resolves. + setTimeout(function () { + window.location.href = (msg.redir || 'index.php?p=admin&c=admins'); + }, 1200); + }; + if (rehashSids && A.SystemRehashAdmins) { + a.call(A.SystemRehashAdmins, { servers: rehashSids }) + .then(navigate) + .catch(navigate); + return; + } + navigate(); + }).catch(function (err) { + // Defensive: sb.api.call doesn't reject on network + // errors (it returns a synthetic envelope), but a + // throw escaping the success callback would otherwise + // leave the submit button busy forever. Per AGENTS.md + // "Loading state on action buttons" — setBusy(btn, + // false) on every non-navigating response branch. + setBusy(submitBtn, false); + toast('error', 'Add admin failed', String(err && err.message ? err.message : err)); }); }); diff --git a/web/themes/default/page_admin_bans_groups.tpl b/web/themes/default/page_admin_bans_groups.tpl index d6fa38a1a..ff2a09b54 100644 --- a/web/themes/default/page_admin_bans_groups.tpl +++ b/web/themes/default/page_admin_bans_groups.tpl @@ -69,7 +69,6 @@ {csrf_field}
diff --git a/web/themes/default/page_admin_mods_add.tpl b/web/themes/default/page_admin_mods_add.tpl index 19b856f58..0592bc922 100644 --- a/web/themes/default/page_admin_mods_add.tpl +++ b/web/themes/default/page_admin_mods_add.tpl @@ -322,6 +322,16 @@ setTimeout(function () { window.location.href = (msg.redir || 'index.php?p=admin&c=mods§ion=list'); }, 800); + }).catch(function (err) { + // #1402 adversarial review MEDIUM 4: defensive .catch() + // arm so a throw escaping the success callback (or a + // sb.api.call internal failure) doesn't leave the + // submit button busy forever. Per AGENTS.md "Loading + // state on action buttons" — setBusy(btn, false) on + // every non-navigating response branch. + setBusy(submitBtn, false); + showMsg('name.msg', String(err && err.message ? err.message : err)); + toast('error', 'Add mod failed', String(err && err.message ? err.message : err)); }); }); })();