fix(1433): kick via panel + kick-on-ban + add server by hostname by rumblefrog · Pull Request #1434 · sbpp/sourcebans-pp

rumblefrog · 2026-05-23T06:28:18Z

Summary

Fixes #1433. The reporter hit three bugs against the v2.0.0 panel:

Kick via web admin panel does nothing — clicking Kick from
the player drawer / ban-detail surface popped the iframe but
every row stayed at "Waiting…" forever.
KickIT does not kick on ban — the post-ban-add RCON
fan-out iframe (same page_kickit.tpl template, just
different entry point) silently no-op'd for identical reasons.
Cannot add a server by hostname — the Add-Server form
advertises "IPv4 / IPv6 / hostname" in its help text but the
JSON handler rejected every hostname-shaped input with
"You must type a valid IP."

Bugs 1 + 2 share a root cause: web/scripts/api.js shipped
endpoint: './api.php' as the load-bearing default. Document-relative
URL resolution lands the fetch on /api.php for top-level panel
renders (the chrome's pages all live at /index.php?p=… so
./api.php resolves against /), but the iframe-routed surfaces
(pages/admin.kickit.php / pages/admin.blockit.php) sit one
directory deep — the iframe's document URL is
/pages/admin.kickit.php, so ./api.php resolved against that lands
on /pages/api.php, which Apache does not rewrite. Every fetch from
the iframe got 404, api.js's fetch resolved to a bad_response-shaped
envelope, and the iframe's load handler short-circuits on !r.ok
with return — the silent early-return path leaving every row at the
initial "Waiting…" text forever. Same code path on every iframe-routed
surface that loads api.js.

Fix: api.js now resolves the endpoint against document.currentScript.src
captured at script-load time. new URL('../api.php', SCRIPT_SRC).href
lands on the panel-root /api.php for both top-level page renders
AND iframe contexts AND subdir installs (https://host/sourcebans/ →
script at …/scripts/api.js → endpoint at …/api.php).

Bug 3: api_servers_add ran FILTER_VALIDATE_IP alone. Now accepts
EITHER a valid IP OR a valid hostname (FILTER_VALIDATE_DOMAIN + FILTER_FLAG_HOSTNAME); paired with a 64-char schema-width gate
because :prefix_servers.ip is VARCHAR(64) NOT NULL and MariaDB
strict mode would otherwise raise SQLSTATE[22001] mid-INSERT,
surfacing as a generic 500 with no audit-log entry. The sibling
admin.edit.server.php validator (which ran a too-loose hand-rolled
^[a-zA-Z0-9.\-]+$ regex) is now byte-for-byte symmetric so the
same value either round-trips through Add AND Edit or fails on both.

Changes

web/scripts/api.js — resolveEndpoint() computes new URL('../api.php', document.currentScript.src).href; bare-relative fallback preserved
for hostile-loader contexts. Source comment documents the static-load
contract (document.currentScript is null under dynamic injection,
which is the exact pre-fix bug shape).
web/api/handlers/servers.php — FILTER_VALIDATE_IP || FILTER_VALIDATE_DOMAIN
(FILTER_FLAG_HOSTNAME) + strlen() > 64 schema-width gate.
web/pages/admin.edit.server.php — same validator pair, same gate;
pre-fix hand-rolled regex replaced.
web/themes/default/page_admin_servers_add.tpl — help text updated
to reflect that hostnames are now actually accepted.
AGENTS.md — paired Anti-pattern entries for
- bare-relative endpoint hardcode (the pre-fix api.js shape)
- dynamic-injection api.js load (would re-open the same bug class)
- FILTER_VALIDATE_IP-alone hostname validation + the too-loose
  hand-rolled hostname regex sibling
- Plus updated "Where to find what" rows for the api.js resolver
  and the shared server-address validator pattern.

Tests

web/tests/api/ServersTest.php — six new / refined tests:
- testAddAcceptsHostname — happy path for simple hostnames.
- testAddAcceptsFqdn — multi-label FQDN (the reporter's marquee
  shape); pins the port column round-trip too.
- testAddAcceptsBareIPv6 — the : chars in 2606:4700:4700::1111
  fail FILTER_FLAG_HOSTNAME, so this pins the
  FILTER_VALIDATE_IP arm of the OR is wired and exercised.
- testAddRefusesDuplicateHostnamePort — hostnames travel through
  the same (ip, port) duplicate-detection code path as IPs.
- testAddRejectsAddressExceedingSchemaWidth — pins the 64-char
  gate's wire-format (validation envelope + 'address' field).
- testAddRejectsWhitespaceInAddress — renamed from the old
  misleading testAddValidatesIpFormat; pins the "fail both
  filters" garbage class.
web/tests/integration/ApiJsEndpointResolutionTest.php — static
shape gate (4 assertions): api.js references
document.currentScript, computes new URL('../api.php', ...),
does NOT bind the bare literal to sb.api.endpoint at
construction time.
web/tests/e2e/specs/flows/kickit-iframe.spec.ts — runtime gate;
loads /pages/admin.kickit.php?check=…&type=0 directly, intercepts
the Actions.KickitLoadServers + Actions.KickitKickPlayer POSTs,
asserts BOTH actions POST to /api.php (NOT /pages/api.php),
and the iframe row transitions past "Waiting…" to "Player not
found." (the well-formed terminal-envelope branch).

Test plan

PHPStan — clean (no errors at level 5)
PHPUnit full suite — 881 tests / 3321 assertions ✓
PHPUnit ServersTest — 41 tests / 181 assertions ✓
PHPUnit ApiJsEndpointResolutionTest — 4/4 ✓
ts-check — clean
api-contract regenerated — no drift
Playwright E2E kickit-iframe — 3/3 on workers=1 (CI shape)
Playwright E2E smoke suite — 62/62

Pre-existing local flakes (server-map-thumbnail, server-player-context-menu,
server-refresh-debounce, admin-groups-server-cards-hydration) reproduce
on main and are unrelated to this PR — they fail against SourceQuery /
live-socket paths under local-stack workers > 1.

Fixes #1433

…mes in servers.add Three bugs from #1433 collapse to two root causes: - Bugs 1 + 2 (kick via panel / kick-on-ban): the iframe-routed kickit and blockit surfaces load /scripts/api.js with a hardcoded `endpoint: './api.php'`. Document-relative resolution against the iframe URL `/pages/admin.kickit.php` lands on /pages/api.php (404), the silent early-return in the iframe template's load handler keeps every row at the initial "Waiting..." text, and the player is never kicked. Resolve the endpoint against `document.currentScript.src` so panel-root + iframe + subdir installs all converge on the same panel-root /api.php. - Bug 3 (hostnames rejected on "Add a server"): api_servers_add ran FILTER_VALIDATE_IP alone despite the form claiming hostname support. Accept FILTER_VALIDATE_DOMAIN+FILTER_FLAG_HOSTNAME as the alternative; the sibling page handler in admin.edit.server.php already accepts hostnames. Form help text updated to match. Adds a static regression guard for api.js's endpoint contract (ApiJsEndpointResolutionTest — asserts document.currentScript + new URL('../api.php' are present, and the bare construction-time literal is gone), a Playwright E2E spec asserting the fetch lands on /api.php (not /pages/api.php), and per-handler ServersTest cases for the hostname / fqdn / garbage-address paths. AGENTS.md gets a one-liner anti-pattern entry + a Where-to-find-what row for web/scripts/api.js.

…tighter tests Adversarial-review follow-up on the prior #1433 fix commit. Pinned the five gaps the reviewer flagged: 1. **Schema-width gate** (BLOCKING). `:prefix_servers.ip` is `VARCHAR(64) NOT NULL` (see `install/includes/sql/struc.sql`), well below RFC 1035's 253-char hostname max. With the prior commit the handler happily accepted a 200-char FQDN, then `INSERT` raised `SQLSTATE[22001] 1406 Data too long` under MariaDB strict mode — the dispatcher's `Throwable` fallback wrapped it as a generic `server_error` 500 with no audit-log entry. Both `api_servers_add` and `admin.edit.server.php` now reject `strlen($ip) > 64` upstream with a structured `validation` envelope / `$validationErrors` entry. Bumping the column to `VARCHAR(255)` is a paired schema-migration follow-up; the gate matches the live column exactly. 2. **Edit-server validator parity**. `admin.edit.server.php` ran a hand-rolled `^[a-zA-Z0-9.\-]+$` regex pre-fix — looser than `FILTER_FLAG_HOSTNAME` (accepted leading hyphens, double dots, IDN-mangled UTF-8) so the same value would round-trip through Edit but get rejected by Add (and vice versa for valid hostnames the JSON handler accepted post-prior-commit). Both surfaces now share the IP || HOSTNAME filter pair so the contract is byte-symmetric. 3. **Test coverage gaps**. Added: - `testAddAcceptsBareIPv6` — `:` characters fail `FILTER_FLAG_HOSTNAME`, so the bare-IPv6 input can ONLY pass via the `FILTER_VALIDATE_IP` arm. Pins both that the IP arm is wired AND that bare-v6 inputs (cloud / dual-stack hosts) work. - `testAddRefusesDuplicateHostnamePort` — pre-fix the duplicate detection ran on `(ip, port)` when only IPs landed there. Hostnames travel through the same code path and must collide on identical `(hostname, port)` too. - `testAddRejectsAddressExceedingSchemaWidth` — pins the 64-char gate's wire-format (validation envelope + `'address'` field). - Renamed the prior `testAddValidatesIpFormat` to `testAddRejectsWhitespaceInAddress` (the misleading name implied IP-format-specific assertions; the test was actually about "fail both filters" garbage input). - Pinned the `port` column round-trip on `testAddAcceptsFqdn` — #1433 is the first hostname-shaped row through the writer, so a future refactor that mis-types-coerces `port` on the hostname-bearing arm would slip past the IP-bearing tests. 4. **api.js docs + static-load contract**. The prior commit's source comment said the `<script>` tag lives in `core/footer.tpl` — wrong; it's in `core/header.tpl` (and `page_kickit.tpl` / `page_blockit.tpl` for the iframe surfaces). Corrected, and inlined the load-bearing static-load requirement (`document.currentScript` is null when the script is appended programmatically — that's the exact pre-#1433 bug shape, so a future refactor that "modernises" to dynamic-injection silently re-opens the regression). 5. **AGENTS.md anti-pattern entry**. Added a paired entry under Anti-patterns for the `FILTER_VALIDATE_IP`-alone shape and the sibling too-loose hand-rolled hostname regex, plus the schema-width gate; updated the existing api.js row to document the static-load contract; added a "Where to find what" row for the shared server-address validator pattern. 6. **Kickit E2E spec tightened**. The post-load assertion now pins that BOTH `kickit.load_servers` AND `kickit.kick_player` POST to `/api.php` (previously just "at least one POST landed on /api.php" — would silently pass on a regression that broke only one of the two actions). Gates: PHPStan ✓ · PHPUnit 881 tests / 3321 assertions ✓ · ServersTest 41 tests / 181 assertions ✓ · ApiJsEndpointResolutionTest 4/4 ✓ · ts-check ✓ · api-contract clean ✓ · kickit-iframe E2E 3/3 on workers=1 ✓ · smoke suite 62/62 ✓. Pre-existing flakes (server-map-thumbnail, server-player-context-menu, server-refresh-debounce, admin-groups-server-cards-hydration) reproduce on `main` and are unrelated to this PR — they fail against SourceQuery / live-socket paths under local-stack `workers > 1`.

rumblefrog added 2 commits May 23, 2026 01:43

rumblefrog added this pull request to the merge queue May 23, 2026

Merged via the queue into main with commit 6cb0c90 May 23, 2026
6 checks passed

rumblefrog deleted the fix/1433-kick-iframe-and-server-hostname branch May 23, 2026 06:38

github-actions Bot locked and limited conversation to collaborators May 23, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(1433): kick via panel + kick-on-ban + add server by hostname#1434

fix(1433): kick via panel + kick-on-ban + add server by hostname#1434
rumblefrog merged 2 commits into
mainfrom
fix/1433-kick-iframe-and-server-hostname

rumblefrog commented May 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

rumblefrog commented May 23, 2026

Summary

Changes

Tests

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant