From e5483d8c4b9e5e6e183d39d910816197697782e7 Mon Sep 17 00:00:00 2001
From: rumblefrog <contact@zhenyangli.me>
Date: Sat, 23 May 2026 01:43:29 -0400
Subject: [PATCH 1/2] fix(1433): resolve api.js endpoint against script URL +
 accept hostnames in servers.add
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 AGENTS.md                                     |  25 ++
 web/api/handlers/servers.php                  |  14 +-
 web/scripts/api.js                            |  41 ++-
 web/tests/api/ServersTest.php                 |  66 ++++-
 .../e2e/specs/flows/kickit-iframe.spec.ts     | 255 ++++++++++++++++++
 .../ApiJsEndpointResolutionTest.php           | 142 ++++++++++
 web/themes/default/page_admin_servers_add.tpl |   2 +-
 7 files changed, 540 insertions(+), 5 deletions(-)
 create mode 100644 web/tests/e2e/specs/flows/kickit-iframe.spec.ts
 create mode 100644 web/tests/integration/ApiJsEndpointResolutionTest.php

diff --git a/AGENTS.md b/AGENTS.md
index 882ec265b..35df26651 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -3115,6 +3115,30 @@ contacting every contributor individually.
   preserves the script's effect doesn't trip this — see "Updater
   migrations" above for the per-script contract.
 - String literals for action names → `Actions.PascalName`.
+- Hardcoding `sb.api.endpoint = './api.php'` (or any other bare
+  document-relative path that depends on the URL of the page loading
+  api.js) → the iframe-routed surfaces
+  (`pages/admin.kickit.php` / `pages/admin.blockit.php`) sit one
+  directory deep, so a document-relative `./api.php` resolves
+  against the iframe's URL to `/pages/api.php` (404 — Apache doesn't
+  rewrite that path) and every kick / block iframe round-trip dies
+  silently with a `bad_response` envelope. The iframe templates'
+  load handlers `return` silently on `!r.ok`, leaving rows at the
+  initial "Waiting…" text forever (#1433 bugs 1 + 2 — kick via panel
+  AND kick-on-ban). `web/scripts/api.js` resolves the endpoint
+  against `document.currentScript.src` instead — `new URL('../api.php',
+  SCRIPT_SRC).href` lands on the panel-root `/api.php` for top-level
+  page renders AND iframe contexts AND subdir installs (`https://host/sourcebans/`).
+  `document.currentScript` is null inside async handlers / promises,
+  so capture the value at IIFE-top before any deferred work.
+  Regression guards: `web/tests/integration/ApiJsEndpointResolutionTest.php`
+  (static — asserts api.js references `document.currentScript` AND
+  `new URL('../api.php'` AND does NOT bind the bare literal to
+  `sb.api.endpoint` at construction time) +
+  `web/tests/e2e/specs/flows/kickit-iframe.spec.ts` (runtime — loads
+  `/pages/admin.kickit.php?check=…&type=0`, asserts the
+  `KickitLoadServers` POST targets `/api.php` NOT `/pages/api.php`,
+  and the iframe rows transition past "Waiting…").
 - Inlining the table prefix → use `:prefix_` and let `Database` rewrite.
 - `htmlspecialchars_decode` / `html_entity_decode` on JSON-API params
   (nickname, reason, chat message, …) → the JSON body is raw UTF-8. The
@@ -3737,6 +3761,7 @@ contacting every contributor individually.
 | Edit a docs page or add a new one (the Astro + Starlight site published at sbpp.github.io) | `docs/src/content/docs/<group>/<slug>.md` (or `.mdx` when the page uses tabs / cards / asides — e.g. `getting-started/quickstart.mdx`, `setup/mariadb.mdx`). New pages also need a sidebar entry in `docs/astro.config.mjs` (the `sidebar:` array). Site config + theme tokens live in `docs/astro.config.mjs` + `docs/src/styles/sbpp.css`. The Starlight chrome ships from `@astrojs/starlight`; layout overrides land under `docs/src/components/` (see `ThemeProvider.astro` for the canonical override shape). Local dev: `cd docs && npm install && npm run dev`. CI gates: `.github/workflows/docs-build.yml` (per-PR build), `docs-deploy-trigger.yml` (main → repository_dispatch into sbpp.github.io), `docs-screenshots.yml` (gated on the `affects-ui` label, runs `docs/scripts/capture.mjs`). Source of truth is here; sbpp.github.io is the deploy shell only (#1333). |
 | Refresh installer / panel screenshots used in docs pages | `docs/scripts/capture.mjs` (Playwright; `npm run capture` in `docs/`). Output lands under `docs/src/assets/auto/{install,panel}/<stable-slug>.png` so docs pages keep referencing the same path across runs. CI does this automatically on PRs labelled `affects-ui`; locally run after `./sbpp.sh up`. STEAM_API_KEY is the all-zero dummy `00000000000000000000000000000000`. |
 | Add a JSON action                      | `web/api/handlers/_register.php` + `web/api/handlers/<topic>.php` |
+| Resolve / override the JSON-API endpoint URL the client-side `sb.api.call(...)` POSTs to | `web/scripts/api.js` (`resolveEndpoint()` — runs once at script-load, computes `new URL('../api.php', document.currentScript.src).href`). The script lives at `/scripts/api.js` regardless of which page loads it, so resolving `../api.php` against the script's own URL lands on the panel-root `/api.php` for top-level page renders, iframe-routed surfaces (`pages/admin.kickit.php` / `pages/admin.blockit.php`), AND subdir installs (`https://host/sourcebans/` → script at `…/scripts/api.js` → endpoint at `…/api.php`). The endpoint stays writable on `sb.api` so callers can swap it; do not edit the resolver to a bare `'./api.php'` literal — that's the pre-#1433 regression shape that 404s every iframe round-trip (`./api.php` resolves against the iframe's document URL `/pages/admin.kickit.php` → `/pages/api.php`, no such route). Pinned by `web/tests/integration/ApiJsEndpointResolutionTest.php` (static) + `web/tests/e2e/specs/flows/kickit-iframe.spec.ts` (runtime). |
 | Add or rename a permission             | `web/configs/permissions/web.json`, then regen contract  |
 | Render a page                          | `web/pages/<page>.php` + `web/includes/View/*View.php`   |
 | Add a new edit page in the admin.edit.* cluster (e.g. `admin.edit.<x>.php`) | `web/pages/admin.edit.<x>.php` (the page handler — thin "validate input, build View, render" shape) + `web/includes/View/AdminEdit<X>View.php` (typed View DTO) + `web/themes/default/page_admin_edit_<x>.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 `<id>.msg` divs, `sbpp_admin_edit_collect_rehash_sids()` for the post-save Rehash Admins step). Anti-patterns to avoid: inline `echo '<form>...'` blocks, `echo '<div id="msg-red">…'` 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['<sentinel>'])` arm. |
diff --git a/web/api/handlers/servers.php b/web/api/handlers/servers.php
index 95a2d4eba..183f2d379 100644
--- a/web/api/handlers/servers.php
+++ b/web/api/handlers/servers.php
@@ -60,8 +60,18 @@ function api_servers_add(array $params): array
     if ($ip === '') {
         throw new ApiError('validation', 'You must type the server address.', 'address');
     }
-    if (!filter_var($ip, FILTER_VALIDATE_IP)) {
-        throw new ApiError('validation', 'You must type a valid IP.', 'address');
+    // Accept EITHER a valid IPv4/IPv6 address OR a valid hostname so the
+    // form's "IPv4 / IPv6 / hostname" help text actually matches behaviour
+    // (#1433). Pre-fix the handler ran FILTER_VALIDATE_IP alone, so any
+    // hostname-shaped address (e.g. `cs.example.com`) was rejected with
+    // "You must type a valid IP." despite the form claiming hostname
+    // support. The sibling page handler `admin.edit.server.php` already
+    // accepts hostnames via a hand-rolled `^[a-zA-Z0-9.\-]+$` regex;
+    // FILTER_VALIDATE_DOMAIN + FILTER_FLAG_HOSTNAME is stricter than that
+    // (rejects leading hyphens, leading dots, etc.) and covers the same
+    // accepted shapes.
+    if (!filter_var($ip, FILTER_VALIDATE_IP) && !filter_var($ip, FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME)) {
+        throw new ApiError('validation', 'You must type a valid IP or hostname.', 'address');
     }
     if ($port === '') {
         throw new ApiError('validation', 'You must type the server port.', 'port');
diff --git a/web/scripts/api.js b/web/scripts/api.js
index 084eaafcb..8673dedc8 100644
--- a/web/scripts/api.js
+++ b/web/scripts/api.js
@@ -27,9 +27,48 @@ to special-case fetch rejections.
         return meta ? (meta.getAttribute('content') || '') : '';
     }
 
+    // Capture the script's own URL at script-load time —
+    // `document.currentScript` is only non-null while the parent
+    // <script> tag is executing synchronously (it's null inside
+    // promises / async handlers / setTimeout callbacks). The
+    // iframe-routed surfaces (`pages/admin.kickit.php`,
+    // `pages/admin.blockit.php`) load api.js from `/scripts/api.js`
+    // regardless of the iframe document's URL, so resolving
+    // `../api.php` against the script's own absolute URL lands on
+    // the panel-root `/api.php` for both top-level page renders AND
+    // iframe contexts (and subdir installs — see `resolveEndpoint`
+    // below).
+    //
+    // Pre-#1433 the endpoint was a bare `./api.php` literal — the
+    // browser resolved it against the *document* URL of whichever
+    // page was hosting the script. For the iframe-routed surfaces
+    // that's `/pages/admin.kickit.php`, so the fetch went to
+    // `/pages/api.php` (404 — Apache doesn't rewrite that), the
+    // iframe's `KickitLoadServers` call resolved to a 404-shaped
+    // `bad_response` envelope, and the load handler's silent
+    // early-return left every row at the initial "Waiting..." text
+    // forever. Player was never kicked. Same code path on every
+    // iframe-routed surface that loads api.js.
+    // `document.currentScript` is typed `HTMLOrSVGScriptElement | null` —
+    // the SVG arm of the union has no `.src`, so cast to HTMLScriptElement
+    // before reading. The runtime guarantee is "the script that's loading
+    // is the `<script src="../scripts/api.js">` tag in `core/footer.tpl`
+    // / `page_kickit.tpl` / `page_blockit.tpl`"; an SVG `<script>` would
+    // never load api.js in the first place.
+    var _cs = /** @type {HTMLScriptElement | null} */ (document.currentScript);
+    var SCRIPT_SRC = (_cs && _cs.src) || '';
+
+    function resolveEndpoint() {
+        if (SCRIPT_SRC) {
+            try { return new URL('../api.php', SCRIPT_SRC).href; }
+            catch (_e) { /* malformed URL — fall through to the bare-relative fallback */ }
+        }
+        return './api.php';
+    }
+
     /** @type {SbApiNamespace} */
     sb.api = {
-        endpoint: './api.php',
+        endpoint: resolveEndpoint(),
 
         async call(action, params) {
             let res;
diff --git a/web/tests/api/ServersTest.php b/web/tests/api/ServersTest.php
index 23641e12e..e4efbb68b 100644
--- a/web/tests/api/ServersTest.php
+++ b/web/tests/api/ServersTest.php
@@ -104,8 +104,72 @@ public function testAddValidatesIp(): void
 
     public function testAddValidatesIpFormat(): void
     {
+        // Post-#1433 the address validator accepts EITHER a valid IP
+        // OR a valid hostname (per `FILTER_VALIDATE_DOMAIN +
+        // FILTER_FLAG_HOSTNAME`) so the help text's "IPv4 / IPv6 /
+        // hostname" claim actually holds. `'not.an.ip'` is a valid
+        // hostname shape now, so use genuinely garbage input here
+        // (embedded whitespace) — both filters reject it.
         $this->loginAsAdmin();
-        $env = $this->api('servers.add', ['ip' => 'not.an.ip', 'port' => '27015', 'mod' => 1, 'group' => '0']);
+        $env = $this->api('servers.add', ['ip' => 'has spaces in it', 'port' => '27015', 'mod' => 1, 'group' => '0']);
+        $this->assertEnvelopeError($env, 'validation');
+        $this->assertSame('address', $env['error']['field']);
+    }
+
+    public function testAddAcceptsHostname(): void
+    {
+        // #1433 — the form template advertises "IPv4 / IPv6 /
+        // hostname" support and the sibling `admin.edit.server.php`
+        // already accepts hostnames; this pins the JSON dispatcher
+        // side onto the same contract.
+        $this->loginAsAdmin();
+        $env = $this->api('servers.add', [
+            'ip'    => 'cs.example.com',
+            'port'  => '27015',
+            'rcon'  => '',
+            'rcon2' => '',
+            'mod'   => 1,
+            'group' => '0',
+        ]);
+        $this->assertTrue($env['ok'], json_encode($env));
+        $row = $this->row('servers', ['sid' => $env['data']['sid']]);
+        $this->assertNotNull($row);
+        $this->assertSame('cs.example.com', $row['ip']);
+        $this->assertSame(27015, (int) $row['port']);
+    }
+
+    public function testAddAcceptsFqdn(): void
+    {
+        // Multi-label FQDN — the reporter's marquee shape on #1433.
+        $this->loginAsAdmin();
+        $env = $this->api('servers.add', [
+            'ip'    => 'gameserver.eu.example.com',
+            'port'  => '27015',
+            'rcon'  => '',
+            'rcon2' => '',
+            'mod'   => 1,
+            'group' => '0',
+        ]);
+        $this->assertTrue($env['ok'], json_encode($env));
+        $row = $this->row('servers', ['sid' => $env['data']['sid']]);
+        $this->assertNotNull($row);
+        $this->assertSame('gameserver.eu.example.com', $row['ip']);
+    }
+
+    public function testAddRejectsGarbageAddress(): void
+    {
+        // Belt-and-suspenders on `testAddValidatesIpFormat` — make
+        // sure obvious garbage (special chars / whitespace) is still
+        // rejected after the hostname pathway opens up.
+        $this->loginAsAdmin();
+        $env = $this->api('servers.add', [
+            'ip'    => 'not a valid hostname or ip',
+            'port'  => '27015',
+            'rcon'  => '',
+            'rcon2' => '',
+            'mod'   => 1,
+            'group' => '0',
+        ]);
         $this->assertEnvelopeError($env, 'validation');
         $this->assertSame('address', $env['error']['field']);
     }
diff --git a/web/tests/e2e/specs/flows/kickit-iframe.spec.ts b/web/tests/e2e/specs/flows/kickit-iframe.spec.ts
new file mode 100644
index 000000000..6e6c8f307
--- /dev/null
+++ b/web/tests/e2e/specs/flows/kickit-iframe.spec.ts
@@ -0,0 +1,255 @@
+/**
+ * Flow: the kickit iframe (`pages/admin.kickit.php`) resolves its
+ * `sb.api.call(...)` fetch against the panel-root `/api.php` (NOT
+ * `/pages/api.php`) — the load-bearing fix for #1433 bugs 1 + 2.
+ *
+ * Background
+ * ----------
+ *
+ * Pre-#1433 `web/scripts/api.js` shipped `endpoint: './api.php'` as
+ * the load-bearing default. Document-relative resolution lands the
+ * fetch on `/api.php` for top-level panel renders (the panel chrome's
+ * pages all live at `/index.php?p=…` so `./api.php` resolves against
+ * `/`), but the iframe-routed surfaces — `pages/admin.kickit.php` and
+ * `pages/admin.blockit.php` — sit one directory deep. The iframe
+ * document URL is `/pages/admin.kickit.php`, so the bare `./api.php`
+ * resolved against the iframe document URL is `/pages/api.php`, which
+ * Apache does not rewrite (`docker/apache/sbpp-prod.conf` exposes the
+ * dispatcher only at `/api.php`). Every fetch from the iframe got
+ * 404, api.js's fetch resolved to a `bad_response`-shaped envelope
+ * (`{ ok: false, error: { code: 'bad_response', … } }`), the iframe's
+ * load handler short-circuits on `!r.ok` with `return` (the silent
+ * early-return path in `page_kickit.tpl`), and every row stayed at
+ * the initial "Waiting…" text forever. Player was never kicked — the
+ * #1433 user-reported symptom.
+ *
+ * Fix
+ * ---
+ *
+ * api.js now resolves the endpoint against
+ * `document.currentScript.src` (captured at script-load time — the
+ * value is null inside async handlers/promises, so the IIFE caches
+ * it at the top). `new URL('../api.php', SCRIPT_SRC).href` lands on
+ * the panel-root `/api.php` for both top-level page renders AND for
+ * iframe contexts AND for subdir installs. The static gate is
+ * `web/tests/integration/ApiJsEndpointResolutionTest.php`; this
+ * runtime gate is the end-to-end contract that proves the algebra
+ * holds in a real browser context.
+ *
+ * What this spec asserts
+ * ----------------------
+ *
+ *  - The kickit iframe (loaded directly at
+ *    `/pages/admin.kickit.php?check=…&type=0`) fires its bootstrap
+ *    `Actions.KickitLoadServers` POST to a URL whose pathname is
+ *    exactly `/api.php` — NOT `/pages/api.php`.
+ *  - The fetch resolves to a real envelope (not 404), so the load
+ *    handler's silent early-return doesn't fire and rows transition
+ *    away from the initial "Waiting…" text. We anchor on the
+ *    `data-testid="kickit-status-0"` row's text change rather than
+ *    on a `setTimeout`-shaped wait (per AGENTS.md
+ *    "Anti-patterns" → no `waitForTimeout`).
+ *
+ * Implementation notes
+ * --------------------
+ *
+ *  - We seed one enabled server via `Actions.ServersAdd` so the
+ *    kickit handler renders at least one row. The seeded IP is in
+ *    the RFC 5737 documentation block (`203.0.113.0/24`); no live
+ *    A2S probe can ever answer, so the kickit RCON path would
+ *    deterministically return `no_connect` — but we stub the
+ *    handler anyway via `page.route` so the spec doesn't depend on
+ *    UDP timing.
+ *  - We intercept BOTH `kickit.load_servers` (the bootstrap call —
+ *    the one whose URL we're proving) AND `kickit.kick_player` (the
+ *    per-row follow-up). Stubbing both keeps the spec deterministic
+ *    and short.
+ *  - Critically, the route matcher is the `/api.php` pathname — we
+ *    WANT to see the URL of every request that lands here. If the
+ *    pre-#1433 regression returned, the request would target
+ *    `/pages/api.php` and our matcher wouldn't fire. So we ALSO
+ *    listen on `page.on('requestfinished', …)` for any POST whose
+ *    URL pathname ends with `/api.php` and assert the captured
+ *    pathname is exactly `/api.php` (not `/pages/api.php`). The
+ *    pair of checks (route fires AND captured URL is panel-root)
+ *    closes the contract from both sides.
+ *  - The page-level `<script>` block calls
+ *    `parent.document.getElementById('dialog-control')` / `srvkicker`
+ *    — both are null when we load the page directly (no parent
+ *    iframe wrapper). The script handles null returns defensively
+ *    so we don't need to scaffold a fake parent.
+ */
+
+import { expect, test } from '../../fixtures/auth.ts';
+import { truncateE2eDb } from '../../fixtures/db.ts';
+
+const KICKIT_URL = '/pages/admin.kickit.php?check=STEAM_0%3A0%3A1&type=0';
+
+test.describe('flow: kickit iframe API call lands on /api.php, not /pages/api.php (#1433)', () => {
+    // Skip on mobile-chromium: the contract is browser-shape-agnostic
+    // (the resolution lives in `web/scripts/api.js` and runs identically
+    // on both form factors) and the second project would double the
+    // truncate-and-reseed traffic against `sourcebans_e2e` per the
+    // pattern documented in `server-refresh-debounce.spec.ts`. Local
+    // dev runs `workers: undefined` and a mid-truncate Apache request
+    // against the shared DB would race.
+    test.beforeEach(({}, testInfo) => {
+        test.skip(
+            testInfo.project.name !== 'chromium',
+            'Browser-shape-agnostic; skip the second project to avoid the truncate-vs-Apache race against sourcebans_e2e.',
+        );
+    });
+
+    test('KickitLoadServers POST targets the panel-root /api.php and rows update', async ({ page }) => {
+        await truncateE2eDb();
+
+        // Seed one enabled server so the kickit handler renders a row.
+        // RFC 5737 documentation IP; no live A2S probe can answer (we
+        // stub the JSON dispatcher response below anyway).
+        await page.goto('/index.php?p=admin&c=servers&section=add');
+        const addEnvelope = await page.evaluate(async () => {
+            const w = window as unknown as {
+                sb: {
+                    api: {
+                        call: (
+                            action: string,
+                            params: Record<string, unknown>,
+                        ) => Promise<{ ok: boolean; data?: { sid?: number } }>;
+                    };
+                };
+                Actions: Record<string, string>;
+            };
+            return await w.sb.api.call(w.Actions.ServersAdd, {
+                ip:      '203.0.113.1',
+                port:    '27015',
+                rcon:    'stub-rcon-password',
+                rcon2:   'stub-rcon-password',
+                // mid=1 (Half-Life 2 DM) is the first row data.sql seeds
+                // into `:prefix_mods`, so it's always available without
+                // a paired mod insert.
+                mod:     1,
+                enabled: true,
+                group:   '0',
+            });
+        });
+        expect(addEnvelope, 'servers.add envelope should round-trip ok').toMatchObject({ ok: true });
+        const sid = (addEnvelope as { data: { sid: number } }).data.sid;
+
+        // Capture every POST to *any* path ending in `/api.php`. The
+        // `requestfinished` listener fires regardless of whether the
+        // request was routed by `page.route` below — pre-#1433 the
+        // request would target `/pages/api.php` and we want to see that
+        // surface here too (so we can assert the contract is "URL ends
+        // with /api.php, NOT /pages/api.php"). If we only listened on
+        // `page.route(**\/api.php**)` we'd miss the regression.
+        const apiPostUrls: string[] = [];
+        page.on('requestfinished', (req) => {
+            if (req.method() !== 'POST') return;
+            const path = new URL(req.url()).pathname;
+            // Match any path that ends in `/api.php` OR `/pages/api.php`
+            // — both are interesting for the contract assertion.
+            if (path.endsWith('/api.php')) {
+                apiPostUrls.push(req.url());
+            }
+        });
+
+        // Stub the JSON dispatcher endpoint so the spec doesn't depend
+        // on a real RCON round-trip. We match on the panel-root
+        // `/api.php` pathname; if the pre-#1433 regression returned,
+        // the iframe's fetch would target `/pages/api.php` and miss
+        // this route entirely — Apache would 404 it and the JS would
+        // see a `bad_response` envelope (the exact pre-fix symptom).
+        // The `requestfinished` listener above catches BOTH paths, so
+        // we'll still see the captured URL and the assertions below
+        // can name which side broke.
+        await page.route(
+            (url) => url.pathname === '/api.php',
+            async (route) => {
+                const req = route.request();
+                if (req.method() !== 'POST') {
+                    await route.continue();
+                    return;
+                }
+                let payload: { action?: string; params?: Record<string, unknown> } = {};
+                try {
+                    payload = JSON.parse(req.postData() ?? '{}');
+                } catch {
+                    await route.continue();
+                    return;
+                }
+
+                if (payload.action === 'kickit.load_servers') {
+                    await route.fulfill({
+                        status: 200,
+                        contentType: 'application/json',
+                        body: JSON.stringify({
+                            ok: true,
+                            data: {
+                                servers: [
+                                    { num: 0, sid, has_rcon: true },
+                                ],
+                            },
+                        }),
+                    });
+                    return;
+                }
+                if (payload.action === 'kickit.kick_player') {
+                    // `not_found` — well-formed terminal envelope; the
+                    // iframe's load handler flips row 0's status text
+                    // to "Player not found." (matches the legacy
+                    // copy in `page_kickit.tpl`'s `processRow` arm).
+                    await route.fulfill({
+                        status: 200,
+                        contentType: 'application/json',
+                        body: JSON.stringify({
+                            ok: true,
+                            data: {
+                                status:   'not_found',
+                                sid,
+                                num:      0,
+                                hostname: 'stub.example.com',
+                                ip:       '203.0.113.1',
+                                port:     '27015',
+                            },
+                        }),
+                    });
+                    return;
+                }
+                await route.continue();
+            },
+        );
+
+        // Navigate directly to the iframe URL. The browser loads
+        // `/pages/admin.kickit.php` as a top-level document, then
+        // pulls in `../scripts/api.js` (resolves to `/scripts/api.js`),
+        // and api.js's `resolveEndpoint()` returns
+        // `http://<host>/api.php` (NOT `http://<host>/pages/api.php`).
+        await page.goto(KICKIT_URL);
+
+        // Anchor on the row's status text transitioning away from
+        // the initial "Waiting…" placeholder. If api.js's endpoint
+        // resolution were broken, this would never fire — the bare
+        // `./api.php` would 404 against `/pages/api.php` and the
+        // iframe's silent early-return would leave the row at
+        // "Waiting…" indefinitely. Playwright auto-waits on the
+        // matcher so no `setTimeout` is needed.
+        const row0Status = page.locator('[data-testid="kickit-status-0"]');
+        await expect(row0Status).toContainText('Player not found.');
+
+        // Both API actions must have round-tripped, and both URLs
+        // must target the panel-root `/api.php`.
+        expect(
+            apiPostUrls.length,
+            `Expected at least 1 POST to */api.php (kickit.load_servers); saw ${apiPostUrls.length}`,
+        ).toBeGreaterThanOrEqual(1);
+
+        for (const url of apiPostUrls) {
+            const path = new URL(url).pathname;
+            expect(
+                path,
+                `Pre-#1433 regression: API call from kickit iframe should target /api.php (panel-root), not ${path}. ` +
+                `If this fires on /pages/api.php, api.js's endpoint resolution has been "simplified" back to the broken literal — see ApiJsEndpointResolutionTest.`,
+            ).toBe('/api.php');
+        }
+    });
+});
diff --git a/web/tests/integration/ApiJsEndpointResolutionTest.php b/web/tests/integration/ApiJsEndpointResolutionTest.php
new file mode 100644
index 000000000..055bb7bca
--- /dev/null
+++ b/web/tests/integration/ApiJsEndpointResolutionTest.php
@@ -0,0 +1,142 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sbpp\Tests\Integration;
+
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Issue #1433: pin the `web/scripts/api.js` endpoint-resolution contract.
+ *
+ * Background
+ * ----------
+ *
+ * Pre-#1433 api.js shipped `endpoint: './api.php'` as the load-bearing
+ * default. Document-relative resolution is fine on a top-level panel
+ * page render (`/index.php?p=…` → resolves against `/` → fetch lands
+ * on `/api.php`), but the iframe-routed surfaces under
+ * `web/pages/admin.kickit.php` + `web/pages/admin.blockit.php` sit
+ * one directory deep — the iframe document URL is
+ * `/pages/admin.kickit.php`, the bare `./api.php` resolves against
+ * the iframe document, and the fetch goes to `/pages/api.php`. The
+ * Apache config (`docker/apache/sbpp-prod.conf`) does not rewrite
+ * `/pages/api.php` so the request 404s; the iframe's
+ * `KickitLoadServers` call resolves to a `bad_response` envelope and
+ * the load handler's silent early-return (`if (!r || !r.ok || !r.data) return`)
+ * leaves every row at the initial "Waiting..." text forever. Player
+ * is never kicked. Same code path on every iframe-routed surface
+ * that loads api.js — kick (Bug 1), post-ban kickit fan-out (Bug 2),
+ * blockit, the future Sleuth iframe, etc.
+ *
+ * Fix
+ * ---
+ *
+ * api.js now captures `document.currentScript.src` at script-load
+ * time (it's null inside async handlers / promises, so the value
+ * has to be cached at the top of the IIFE) and computes the
+ * endpoint as `new URL('../api.php', SCRIPT_SRC).href`. This lands
+ * on the panel-root `/api.php` for top-level renders AND for
+ * iframe contexts AND for subdir installs (`https://host/sourcebans/`
+ * → script at `…/sourcebans/scripts/api.js` → endpoint at
+ * `…/sourcebans/api.php`).
+ *
+ * Static gate
+ * -----------
+ *
+ * This test reads `web/scripts/api.js` as a string and asserts:
+ *
+ *   - The literal `document.currentScript` is referenced (the
+ *     load-bearing capture mechanism).
+ *   - The literal `new URL('../api.php'` appears (the resolution
+ *     call). The relative-segment count is part of the contract —
+ *     api.js lives at `/scripts/api.js`, so `../api.php` is the
+ *     only correct shape; `./api.php` from the script URL would
+ *     land on `/scripts/api.php` (also 404).
+ *   - The bare `endpoint: './api.php'` literal is NOT the
+ *     load-bearing default. It can survive as a fallback string
+ *     literal (the IIFE keeps it for the no-`currentScript` path,
+ *     e.g. ancient browsers without `document.currentScript`
+ *     support), but it must NOT be the value `sb.api.endpoint`
+ *     binds to at construction time. We approximate "is the
+ *     load-bearing default" by asserting `endpoint: './api.php'`
+ *     does NOT appear: the post-fix code reads `endpoint: resolveEndpoint(),`
+ *     so the literal-with-init shape is gone.
+ *
+ * This catches a future refactor that "simplifies" the endpoint
+ * back to the broken literal (the kind of edit a reviewer might
+ * wave through if they didn't know the iframe-routed surfaces
+ * exist).
+ *
+ * Pure file scanning — extends `PHPUnit\Framework\TestCase` directly
+ * (no DB / session / Smarty bring-up). Sister of
+ * `DeadJsCallSitesTest`; same per-file forbidden-substring shape,
+ * inverted into per-required + per-forbidden assertions for the
+ * single api.js surface.
+ */
+final class ApiJsEndpointResolutionTest extends TestCase
+{
+    private static function apiJsPath(): string
+    {
+        return ROOT . 'scripts/api.js';
+    }
+
+    private static function apiJsContents(): string
+    {
+        $contents = @file_get_contents(self::apiJsPath());
+        if ($contents === false) {
+            self::fail('Could not read ' . self::apiJsPath() . ' — file moved or test bootstrap broke?');
+        }
+        return $contents;
+    }
+
+    public function testApiJsExists(): void
+    {
+        $this->assertFileExists(
+            self::apiJsPath(),
+            'web/scripts/api.js must live at scripts/api.js for the iframe-routed surfaces (kickit / blockit) to load `../scripts/api.js` correctly. If you moved it, update both this test AND the per-template `<script src="">` tags in `page_kickit.tpl` / `page_blockit.tpl` / the panel chrome footer.',
+        );
+    }
+
+    public function testApiJsReferencesDocumentCurrentScript(): void
+    {
+        $contents = self::apiJsContents();
+        $this->assertStringContainsString(
+            'document.currentScript',
+            $contents,
+            "api.js must capture `document.currentScript.src` to resolve the endpoint against the script's own absolute URL (#1433). Without this the endpoint falls back to a document-relative path and the iframe-routed surfaces (pages/admin.kickit.php, pages/admin.blockit.php) silently 404 on every API call.",
+        );
+    }
+
+    public function testApiJsResolvesEndpointAgainstScriptSrc(): void
+    {
+        $contents = self::apiJsContents();
+        $this->assertStringContainsString(
+            "new URL('../api.php'",
+            $contents,
+            "api.js must compute the endpoint as `new URL('../api.php', SCRIPT_SRC).href` (#1433). The `..` segment is load-bearing — api.js lives at `/scripts/api.js`, so `./api.php` resolves against the script URL to `/scripts/api.php` (also 404). If you intentionally changed the resolution shape, update this assertion + verify the iframe-routed surfaces still hit `/api.php`.",
+        );
+    }
+
+    public function testApiJsDoesNotHardcodeRelativeEndpoint(): void
+    {
+        // The fallback string `'./api.php'` may legitimately survive
+        // as the no-`currentScript` defensive return value inside
+        // `resolveEndpoint()`. What CANNOT survive is the
+        // pre-#1433 shape that bound the literal directly to
+        // `sb.api.endpoint` at construction time:
+        //
+        //     endpoint: './api.php',
+        //
+        // Asserting against the exact construction-time bind shape
+        // (`endpoint: './api.php',`) catches a refactor that
+        // "simplifies" the endpoint back to the broken literal
+        // without false-matching the legitimate fallback return.
+        $contents = self::apiJsContents();
+        $this->assertStringNotContainsString(
+            "endpoint: './api.php',",
+            $contents,
+            "api.js must NOT bind `sb.api.endpoint` to the bare relative literal `'./api.php'` at construction time (#1433). The post-fix shape is `endpoint: resolveEndpoint(),`. If you reverted the resolver, every iframe-routed surface (kick / block / kick-on-ban) silently 404s.",
+        );
+    }
+}
diff --git a/web/themes/default/page_admin_servers_add.tpl b/web/themes/default/page_admin_servers_add.tpl
index c73e954cb..8c3f89ee7 100644
--- a/web/themes/default/page_admin_servers_add.tpl
+++ b/web/themes/default/page_admin_servers_add.tpl
@@ -53,7 +53,7 @@
                                placeholder="203.0.113.10"
                                required
                                data-testid="addserver-address">
-                        <span class="text-xs text-muted">IPv4 / IPv6 / hostname. The dispatcher validates with PHP's <code class="font-mono">FILTER_VALIDATE_IP</code>.</span>
+                        <span class="text-xs text-muted">IPv4 / IPv6 / hostname. Hostnames are resolved by the game server at connect time.</span>
                     </label>
                     <label class="block">
                         <span class="label">Server port</span>

From 37b0b6d911a06bc76f0c47dc1ba63e663467bacf Mon Sep 17 00:00:00 2001
From: rumblefrog <contact@zhenyangli.me>
Date: Sat, 23 May 2026 02:27:31 -0400
Subject: [PATCH 2/2] =?UTF-8?q?fix(1433):=20address=20review=20=E2=80=94?=
 =?UTF-8?q?=20schema-width=20gate=20+=20edit-server=20parity=20+=20tighter?=
 =?UTF-8?q?=20tests?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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`.
---
 AGENTS.md                                     |  56 ++++++++-
 web/api/handlers/servers.php                  |  27 ++++-
 web/pages/admin.edit.server.php               |  17 ++-
 web/scripts/api.js                            |  21 +++-
 web/tests/api/ServersTest.php                 | 108 +++++++++++++++++-
 .../e2e/specs/flows/kickit-iframe.spec.ts     |  35 +++++-
 6 files changed, 240 insertions(+), 24 deletions(-)

diff --git a/AGENTS.md b/AGENTS.md
index 35df26651..699688a18 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -3139,6 +3139,59 @@ contacting every contributor individually.
   `/pages/admin.kickit.php?check=…&type=0`, asserts the
   `KickitLoadServers` POST targets `/api.php` NOT `/pages/api.php`,
   and the iframe rows transition past "Waiting…").
+- Loading `web/scripts/api.js` via dynamic injection
+  (`document.createElement('script')`, `<script>document.write(...)</script>`,
+  async loaders, ES-module `import()`, third-party bundlers) → the IIFE
+  reads `document.currentScript.src` at script-load time to compute the
+  endpoint URL, but `document.currentScript` is `null` when a script
+  is appended programmatically (it's only non-null while the parent
+  static `<script>` tag is executing synchronously). With a `null`
+  currentScript the `SCRIPT_SRC` constant collapses to the empty
+  string and `resolveEndpoint()` falls through to the bare-relative
+  `./api.php` fallback — i.e. the exact pre-#1433 regression shape.
+  api.js MUST be loaded via one of the three static `<script src="…">`
+  tags in the default theme (`core/header.tpl` top-level chrome →
+  `./scripts/api.js`, `page_kickit.tpl` / `page_blockit.tpl` iframe
+  surfaces → `../scripts/api.js`). A theme fork that wants to lazy-
+  load api.js (e.g. defer until first `sb.api.call`) needs to ship
+  its own paired endpoint resolver — not reuse the in-tree IIFE.
+- `filter_var($address, FILTER_VALIDATE_IP)` ALONE for a server-
+  address input (`api_servers_add`, `admin.edit.server.php`'s
+  `address` field) → the form templates advertise "IPv4 / IPv6 /
+  hostname" but the IP-only validator rejects every hostname-shaped
+  input with "You must type a valid IP." (#1433 bug 3 — reporter
+  could not add `cs.example.com`). The contract is to accept EITHER
+  a valid IP OR a valid hostname:
+  `filter_var($x, FILTER_VALIDATE_IP) || filter_var($x, FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME)`
+  on both the JSON dispatcher (`web/api/handlers/servers.php`) and
+  the page handler (`web/pages/admin.edit.server.php`). The hand-rolled
+  `^[a-zA-Z0-9.\-]+$` regex that lived in `admin.edit.server.php`
+  pre-#1433 is the sibling anti-pattern: it accepted shapes the IP
+  filter rejects (leading hyphens, double dots, IDN-mangled UTF-8)
+  AND was looser than `FILTER_FLAG_HOSTNAME`'s real RFC 1035 check —
+  the two surfaces silently disagreed on what's accepted. Both now
+  share the IP || HOSTNAME filter pair so a value either round-trips
+  through Add AND Edit or fails on both. Schema width gate: the
+  `:prefix_servers.ip` column is `VARCHAR(64) NOT NULL` (see
+  `web/install/includes/sql/struc.sql`), well below RFC 1035's 253-char
+  hostname max, so both surfaces ALSO carry an explicit `strlen($ip) > 64`
+  validation step that emits a structured `validation` envelope (or
+  the page-handler-equivalent `$validationErrors['address']`)
+  instead of letting MariaDB strict mode raise `SQLSTATE[22001] 1406
+  Data too long for column 'ip'` mid-INSERT — the bare PDOException
+  would surface as a generic 500 with no actionable copy AND skip the
+  audit-log entry. Bumping the column to `VARCHAR(255)` is a paired
+  schema-migration follow-up (out of #1433's scope). Regression
+  guards: `web/tests/api/ServersTest.php::testAddAcceptsHostname` /
+  `testAddAcceptsFqdn` / `testAddAcceptsBareIPv6` (the `:` characters
+  pin the FILTER_VALIDATE_IP arm — `FILTER_FLAG_HOSTNAME` rejects them
+  outright, so a bare-IPv6 round-trip can only succeed when the IP
+  branch of the OR is wired and exercised) /
+  `testAddRejectsAddressExceedingSchemaWidth` /
+  `testAddRefusesDuplicateHostnamePort` /
+  `testAddRejectsWhitespaceInAddress` (representative of the "fail
+  both filters" garbage class — also pinned by
+  `testAddRejectsGarbageAddress`).
 - Inlining the table prefix → use `:prefix_` and let `Database` rewrite.
 - `htmlspecialchars_decode` / `html_entity_decode` on JSON-API params
   (nickname, reason, chat message, …) → the JSON body is raw UTF-8. The
@@ -3761,7 +3814,8 @@ contacting every contributor individually.
 | Edit a docs page or add a new one (the Astro + Starlight site published at sbpp.github.io) | `docs/src/content/docs/<group>/<slug>.md` (or `.mdx` when the page uses tabs / cards / asides — e.g. `getting-started/quickstart.mdx`, `setup/mariadb.mdx`). New pages also need a sidebar entry in `docs/astro.config.mjs` (the `sidebar:` array). Site config + theme tokens live in `docs/astro.config.mjs` + `docs/src/styles/sbpp.css`. The Starlight chrome ships from `@astrojs/starlight`; layout overrides land under `docs/src/components/` (see `ThemeProvider.astro` for the canonical override shape). Local dev: `cd docs && npm install && npm run dev`. CI gates: `.github/workflows/docs-build.yml` (per-PR build), `docs-deploy-trigger.yml` (main → repository_dispatch into sbpp.github.io), `docs-screenshots.yml` (gated on the `affects-ui` label, runs `docs/scripts/capture.mjs`). Source of truth is here; sbpp.github.io is the deploy shell only (#1333). |
 | Refresh installer / panel screenshots used in docs pages | `docs/scripts/capture.mjs` (Playwright; `npm run capture` in `docs/`). Output lands under `docs/src/assets/auto/{install,panel}/<stable-slug>.png` so docs pages keep referencing the same path across runs. CI does this automatically on PRs labelled `affects-ui`; locally run after `./sbpp.sh up`. STEAM_API_KEY is the all-zero dummy `00000000000000000000000000000000`. |
 | Add a JSON action                      | `web/api/handlers/_register.php` + `web/api/handlers/<topic>.php` |
-| Resolve / override the JSON-API endpoint URL the client-side `sb.api.call(...)` POSTs to | `web/scripts/api.js` (`resolveEndpoint()` — runs once at script-load, computes `new URL('../api.php', document.currentScript.src).href`). The script lives at `/scripts/api.js` regardless of which page loads it, so resolving `../api.php` against the script's own URL lands on the panel-root `/api.php` for top-level page renders, iframe-routed surfaces (`pages/admin.kickit.php` / `pages/admin.blockit.php`), AND subdir installs (`https://host/sourcebans/` → script at `…/scripts/api.js` → endpoint at `…/api.php`). The endpoint stays writable on `sb.api` so callers can swap it; do not edit the resolver to a bare `'./api.php'` literal — that's the pre-#1433 regression shape that 404s every iframe round-trip (`./api.php` resolves against the iframe's document URL `/pages/admin.kickit.php` → `/pages/api.php`, no such route). Pinned by `web/tests/integration/ApiJsEndpointResolutionTest.php` (static) + `web/tests/e2e/specs/flows/kickit-iframe.spec.ts` (runtime). |
+| Resolve / override the JSON-API endpoint URL the client-side `sb.api.call(...)` POSTs to | `web/scripts/api.js` (`resolveEndpoint()` — runs once at script-load, computes `new URL('../api.php', document.currentScript.src).href`). The script lives at `/scripts/api.js` regardless of which page loads it, so resolving `../api.php` against the script's own URL lands on the panel-root `/api.php` for top-level page renders, iframe-routed surfaces (`pages/admin.kickit.php` / `pages/admin.blockit.php`), AND subdir installs (`https://host/sourcebans/` → script at `…/scripts/api.js` → endpoint at `…/api.php`). The endpoint stays writable on `sb.api` so callers can swap it; do not edit the resolver to a bare `'./api.php'` literal — that's the pre-#1433 regression shape that 404s every iframe round-trip (`./api.php` resolves against the iframe's document URL `/pages/admin.kickit.php` → `/pages/api.php`, no such route). **Load via static `<script src="…">` only** — `document.currentScript` is `null` when the script is appended programmatically (`document.createElement('script')`, `<script>document.write(...)</script>`, async loaders, ES-module `import()`), and a null `currentScript` collapses `SCRIPT_SRC` to the empty string and silently falls back to the bare-relative `./api.php` — i.e. the exact pre-#1433 bug. The three static load sites in the default theme are `core/header.tpl` (top-level panel chrome → `./scripts/api.js`), `page_kickit.tpl`, and `page_blockit.tpl` (iframe surfaces → `../scripts/api.js`); a theme fork that wants to lazy-load needs its own paired endpoint resolver. Pinned by `web/tests/integration/ApiJsEndpointResolutionTest.php` (static) + `web/tests/e2e/specs/flows/kickit-iframe.spec.ts` (runtime). |
+| Validate a server-address input (IPv4 / IPv6 / hostname) on either Add-Server (`api_servers_add`) or Edit-Server (`admin.edit.server.php`) | Shared validator pattern: `filter_var($x, FILTER_VALIDATE_IP) \|\| filter_var($x, FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME)`. Both surfaces MUST share the same predicate so a value either round-trips through Add AND Edit or fails on both (#1433); pre-fix the JSON dispatcher ran IP-only while the page handler ran a too-loose hand-rolled `^[a-zA-Z0-9.\-]+$` regex. The matching schema-width gate (`strlen($x) > 64`, surfacing as `validation` envelope or `$validationErrors['address']`) is paired with the IP/hostname check because `:prefix_servers.ip` is `VARCHAR(64) NOT NULL` (see `web/install/includes/sql/struc.sql`) — well below RFC 1035's 253-char hostname max, and MariaDB strict mode would otherwise raise `SQLSTATE[22001] 1406 Data too long for column 'ip'` mid-INSERT (generic 500 + no audit-log entry). Bumping the column to `VARCHAR(255)` is a paired schema-migration follow-up. Pinned by `web/tests/api/ServersTest.php::testAddAcceptsHostname` / `testAddAcceptsFqdn` / `testAddAcceptsBareIPv6` / `testAddRejectsAddressExceedingSchemaWidth` / `testAddRefusesDuplicateHostnamePort` / `testAddRejectsWhitespaceInAddress`. |
 | Add or rename a permission             | `web/configs/permissions/web.json`, then regen contract  |
 | Render a page                          | `web/pages/<page>.php` + `web/includes/View/*View.php`   |
 | Add a new edit page in the admin.edit.* cluster (e.g. `admin.edit.<x>.php`) | `web/pages/admin.edit.<x>.php` (the page handler — thin "validate input, build View, render" shape) + `web/includes/View/AdminEdit<X>View.php` (typed View DTO) + `web/themes/default/page_admin_edit_<x>.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 `<id>.msg` divs, `sbpp_admin_edit_collect_rehash_sids()` for the post-save Rehash Admins step). Anti-patterns to avoid: inline `echo '<form>...'` blocks, `echo '<div id="msg-red">…'` 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['<sentinel>'])` arm. |
diff --git a/web/api/handlers/servers.php b/web/api/handlers/servers.php
index 183f2d379..6f6a01ee1 100644
--- a/web/api/handlers/servers.php
+++ b/web/api/handlers/servers.php
@@ -65,14 +65,31 @@ function api_servers_add(array $params): array
     // (#1433). Pre-fix the handler ran FILTER_VALIDATE_IP alone, so any
     // hostname-shaped address (e.g. `cs.example.com`) was rejected with
     // "You must type a valid IP." despite the form claiming hostname
-    // support. The sibling page handler `admin.edit.server.php` already
-    // accepts hostnames via a hand-rolled `^[a-zA-Z0-9.\-]+$` regex;
-    // FILTER_VALIDATE_DOMAIN + FILTER_FLAG_HOSTNAME is stricter than that
-    // (rejects leading hyphens, leading dots, etc.) and covers the same
-    // accepted shapes.
+    // support. FILTER_VALIDATE_DOMAIN + FILTER_FLAG_HOSTNAME is stricter
+    // than `admin.edit.server.php`'s pre-fix hand-rolled `^[a-zA-Z0-9.\-]+$`
+    // regex (rejects leading hyphens, leading dots, double-dots, etc.)
+    // and covers the same accepted shapes. The page-handler sibling now
+    // uses the same filter pair so the two surfaces accept identical input.
     if (!filter_var($ip, FILTER_VALIDATE_IP) && !filter_var($ip, FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME)) {
         throw new ApiError('validation', 'You must type a valid IP or hostname.', 'address');
     }
+    // Schema width gate. `:prefix_servers.ip` is `VARCHAR(64) NOT NULL`
+    // (see `web/install/includes/sql/struc.sql`), and MariaDB 10.x's
+    // default `sql_mode` includes `STRICT_TRANS_TABLES` — an INSERT
+    // with a >64-char hostname raises `SQLSTATE[22001] 1406 Data too
+    // long for column 'ip'`. The PDOException escapes the handler and
+    // the dispatcher's `Throwable` fallback wraps it as a generic
+    // `server_error` 500, so the operator sees "An unexpected error
+    // occurred" instead of an actionable `validation` envelope AND the
+    // audit-log entry below never runs. RFC 1035 caps a full FQDN at
+    // 253 chars; some cloud DNS shapes (`*.cloudapp.azure.com`, k8s
+    // service DNS, long SaaS gameserver hostnames) sit well past 64.
+    // The 64-char gate here matches the column exactly so the rejection
+    // surfaces as a clean validation error with a clear cap; bumping
+    // the column to `VARCHAR(255)` is a paired schema-migration follow-up.
+    if (strlen($ip) > 64) {
+        throw new ApiError('validation', 'Server address must be at most 64 characters.', 'address');
+    }
     if ($port === '') {
         throw new ApiError('validation', 'You must type the server port.', 'port');
     }
diff --git a/web/pages/admin.edit.server.php b/web/pages/admin.edit.server.php
index 87bf6578e..55937a0f8 100644
--- a/web/pages/admin.edit.server.php
+++ b/web/pages/admin.edit.server.php
@@ -78,8 +78,23 @@
     if ($rawAddress === '') {
         $validationErrors['address'] = 'You must type the server address.';
     } elseif (!filter_var($rawAddress, FILTER_VALIDATE_IP)
-        && !preg_match('/^[a-zA-Z0-9.\\-]+$/', $rawAddress)) {
+        && !filter_var($rawAddress, FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME)) {
+        // #1433 follow-up — keep this validator byte-for-byte symmetric
+        // with `api_servers_add` (the JSON dispatcher). Pre-fix this
+        // surface ran a hand-rolled `^[a-zA-Z0-9.\-]+$` regex which
+        // accepted shapes the JSON handler rejects (leading hyphens,
+        // double dots, etc.), so the same value would be accepted via
+        // Edit but rejected via Add (and vice versa for valid hostnames
+        // before the JSON handler grew its `FILTER_VALIDATE_DOMAIN`
+        // arm). Both surfaces now share the IP || HOSTNAME filter pair.
         $validationErrors['address'] = 'You must type a valid IP or hostname.';
+    } elseif (strlen($rawAddress) > 64) {
+        // Schema width gate — see the matching comment in
+        // `web/api/handlers/servers.php::api_servers_add`. The column
+        // is `VARCHAR(64)` and MariaDB strict mode would otherwise
+        // surface a generic 500 with no actionable copy AND skip the
+        // audit-log entry below.
+        $validationErrors['address'] = 'Server address must be at most 64 characters.';
     }
 
     if ($rawPort === '' || !ctype_digit($rawPort)
diff --git a/web/scripts/api.js b/web/scripts/api.js
index 8673dedc8..0613c3d9a 100644
--- a/web/scripts/api.js
+++ b/web/scripts/api.js
@@ -39,6 +39,21 @@ to special-case fetch rejections.
     // iframe contexts (and subdir installs — see `resolveEndpoint`
     // below).
     //
+    // **api.js MUST be loaded via a static `<script src="…">` tag,
+    // never via dynamic injection** (`document.createElement('script')`,
+    // `<script>document.write(...)</script>`, async loaders). Dynamic
+    // injection runs the script with `document.currentScript === null`,
+    // which collapses `SCRIPT_SRC` to the empty string and silently
+    // falls back to the bare-relative `./api.php` endpoint — i.e.
+    // the exact pre-#1433 bug shape. The runtime contract is "the
+    // script that's loading is one of the three static `<script>` tags
+    // in `core/header.tpl` (top-level panel chrome,
+    // `./scripts/api.js`), `page_kickit.tpl` / `page_blockit.tpl`
+    // (iframe surfaces, `../scripts/api.js`)". An SVG `<script>`
+    // would never load api.js in the first place, so the
+    // `HTMLOrSVGScriptElement` union's SVG arm (which lacks `.src`)
+    // is unreachable; we cast to `HTMLScriptElement` to read `.src`.
+    //
     // Pre-#1433 the endpoint was a bare `./api.php` literal — the
     // browser resolved it against the *document* URL of whichever
     // page was hosting the script. For the iframe-routed surfaces
@@ -49,12 +64,6 @@ to special-case fetch rejections.
     // early-return left every row at the initial "Waiting..." text
     // forever. Player was never kicked. Same code path on every
     // iframe-routed surface that loads api.js.
-    // `document.currentScript` is typed `HTMLOrSVGScriptElement | null` —
-    // the SVG arm of the union has no `.src`, so cast to HTMLScriptElement
-    // before reading. The runtime guarantee is "the script that's loading
-    // is the `<script src="../scripts/api.js">` tag in `core/footer.tpl`
-    // / `page_kickit.tpl` / `page_blockit.tpl`"; an SVG `<script>` would
-    // never load api.js in the first place.
     var _cs = /** @type {HTMLScriptElement | null} */ (document.currentScript);
     var SCRIPT_SRC = (_cs && _cs.src) || '';
 
diff --git a/web/tests/api/ServersTest.php b/web/tests/api/ServersTest.php
index e4efbb68b..5d4c5dab1 100644
--- a/web/tests/api/ServersTest.php
+++ b/web/tests/api/ServersTest.php
@@ -102,14 +102,26 @@ public function testAddValidatesIp(): void
         $this->assertSnapshot('servers/add_validation_address', $env);
     }
 
-    public function testAddValidatesIpFormat(): void
+    public function testAddRejectsWhitespaceInAddress(): void
     {
         // Post-#1433 the address validator accepts EITHER a valid IP
         // OR a valid hostname (per `FILTER_VALIDATE_DOMAIN +
         // FILTER_FLAG_HOSTNAME`) so the help text's "IPv4 / IPv6 /
-        // hostname" claim actually holds. `'not.an.ip'` is a valid
-        // hostname shape now, so use genuinely garbage input here
-        // (embedded whitespace) — both filters reject it.
+        // hostname" claim actually holds. Embedded whitespace is one
+        // shape BOTH filters reject — pin it as a representative
+        // garbage-input case. The contract: validation MUST reject
+        // values neither filter accepts.
+        //
+        // Note: PHP's `FILTER_FLAG_HOSTNAME` accepts numeric-only
+        // labels (`999.999.999.999` is a "valid hostname" per the
+        // RFC 1035 label grammar — `4.3.2.1.in-addr.arpa` is the
+        // canonical PTR shape), so the IP-rejection contract can't
+        // be tested by feeding "obviously wrong" IPs and expecting
+        // the validator to surface them as IP errors specifically.
+        // The IP path is instead pinned by `testAddAcceptsBareIPv6`
+        // — the `:` characters in `2606:4700:4700::1111` are
+        // rejected by `FILTER_FLAG_HOSTNAME`, so that input can
+        // ONLY round-trip via the `FILTER_VALIDATE_IP` arm.
         $this->loginAsAdmin();
         $env = $this->api('servers.add', ['ip' => 'has spaces in it', 'port' => '27015', 'mod' => 1, 'group' => '0']);
         $this->assertEnvelopeError($env, 'validation');
@@ -154,12 +166,96 @@ public function testAddAcceptsFqdn(): void
         $row = $this->row('servers', ['sid' => $env['data']['sid']]);
         $this->assertNotNull($row);
         $this->assertSame('gameserver.eu.example.com', $row['ip']);
+        // Pin the port column too — the snapshot suite already
+        // covers IPv4-route happy paths but #1433 is the first
+        // hostname-shaped row through the writer; a future refactor
+        // that incorrectly typed-coerces `port` against the
+        // hostname-bearing arm would slip past the IP-bearing tests.
+        $this->assertSame(27015, (int) $row['port']);
+    }
+
+    public function testAddAcceptsBareIPv6(): void
+    {
+        // FILTER_VALIDATE_IP accepts both v4 and v6; FILTER_FLAG_NO_PRIV_RANGE
+        // / NO_RES_RANGE are NOT set, so private+reserved blocks are
+        // fine for self-hosters running game servers inside their LAN.
+        // Pin a public IPv6 (Cloudflare DNS) to keep the shape simple.
+        // The `:port` column is separate so no `[v6]:port` parsing is
+        // needed at this layer.
+        $this->loginAsAdmin();
+        $env = $this->api('servers.add', [
+            'ip'    => '2606:4700:4700::1111',
+            'port'  => '27015',
+            'rcon'  => '',
+            'rcon2' => '',
+            'mod'   => 1,
+            'group' => '0',
+        ]);
+        $this->assertTrue($env['ok'], json_encode($env));
+        $row = $this->row('servers', ['sid' => $env['data']['sid']]);
+        $this->assertNotNull($row);
+        $this->assertSame('2606:4700:4700::1111', $row['ip']);
+    }
+
+    public function testAddRejectsAddressExceedingSchemaWidth(): void
+    {
+        // `:prefix_servers.ip` is `VARCHAR(64) NOT NULL`. Pre-#1433
+        // the implicit cap was "IPv4 fits, hostnames don't exist
+        // here" — `FILTER_VALIDATE_IP` rejected everything over 39
+        // chars (the IPv6 max). Post-#1433 hostnames up to RFC 1035's
+        // 253-char max pass `FILTER_VALIDATE_DOMAIN |
+        // FILTER_FLAG_HOSTNAME`, but the column would truncate
+        // anything >64 and MariaDB's `STRICT_TRANS_TABLES` mode
+        // (the default) would raise `SQLSTATE[22001]` — surfacing
+        // as a generic `server_error` 500 with no audit-log entry.
+        // The handler-side gate translates that into an actionable
+        // `validation` envelope with the same shape the empty-input
+        // / bad-format branches use.
+        $this->loginAsAdmin();
+        // 64 chars at a glance: 13 chars × 5 = 65; subtract one for
+        // the trailing `.com` (4 chars) and add a single leading
+        // `a` to land on 65 chars — one over the cap.
+        $long = str_repeat('a', 61) . '.com'; // 65 chars total
+        $this->assertGreaterThan(64, strlen($long));
+        $env = $this->api('servers.add', [
+            'ip'    => $long,
+            'port'  => '27015',
+            'rcon'  => '',
+            'rcon2' => '',
+            'mod'   => 1,
+            'group' => '0',
+        ]);
+        $this->assertEnvelopeError($env, 'validation');
+        $this->assertSame('address', $env['error']['field']);
+        $this->assertStringContainsString('64', $env['error']['message'],
+            'validation copy must surface the 64-char cap so the operator knows what to trim',
+        );
+    }
+
+    public function testAddRefusesDuplicateHostnamePort(): void
+    {
+        // #1433 follow-up — the duplicate-detection branch
+        // (the existing `INSERT IGNORE`-style guard further down
+        // in `api_servers_add`) keyed off `(ip, port)` pre-#1433
+        // when only IPs landed in that column. Hostnames travel
+        // through the same code path and must collide on identical
+        // `(hostname, port)` too. Mirror of
+        // `testAddRefusesDuplicateIpPort`.
+        $this->loginAsAdmin();
+        $params = [
+            'ip' => 'cs2.example.com', 'port' => '27015',
+            'rcon' => '', 'rcon2' => '', 'mod' => 1, 'group' => '0',
+        ];
+        $first = $this->api('servers.add', $params);
+        $this->assertTrue($first['ok'], json_encode($first));
+        $env = $this->api('servers.add', $params);
+        $this->assertEnvelopeError($env, 'duplicate');
     }
 
     public function testAddRejectsGarbageAddress(): void
     {
-        // Belt-and-suspenders on `testAddValidatesIpFormat` — make
-        // sure obvious garbage (special chars / whitespace) is still
+        // Belt-and-suspenders — make sure obvious garbage
+        // (special chars / whitespace / shell-meta) is still
         // rejected after the hostname pathway opens up.
         $this->loginAsAdmin();
         $env = $this->api('servers.add', [
diff --git a/web/tests/e2e/specs/flows/kickit-iframe.spec.ts b/web/tests/e2e/specs/flows/kickit-iframe.spec.ts
index 6e6c8f307..a5ff95f7c 100644
--- a/web/tests/e2e/specs/flows/kickit-iframe.spec.ts
+++ b/web/tests/e2e/specs/flows/kickit-iframe.spec.ts
@@ -142,7 +142,15 @@ test.describe('flow: kickit iframe API call lands on /api.php, not /pages/api.ph
         // surface here too (so we can assert the contract is "URL ends
         // with /api.php, NOT /pages/api.php"). If we only listened on
         // `page.route(**\/api.php**)` we'd miss the regression.
+        // We also capture the action name from the JSON body so the
+        // post-load assertions can pin BOTH expected calls
+        // (`kickit.load_servers` and `kickit.kick_player`) rather
+        // than just "at least one POST to /api.php landed", which
+        // would silently pass on a regression where load_servers
+        // succeeded but kick_player went to /pages/api.php (or
+        // vice versa).
         const apiPostUrls: string[] = [];
+        const apiActions = new Set<string>();
         page.on('requestfinished', (req) => {
             if (req.method() !== 'POST') return;
             const path = new URL(req.url()).pathname;
@@ -150,6 +158,16 @@ test.describe('flow: kickit iframe API call lands on /api.php, not /pages/api.ph
             // — both are interesting for the contract assertion.
             if (path.endsWith('/api.php')) {
                 apiPostUrls.push(req.url());
+                try {
+                    const body = JSON.parse(req.postData() ?? '{}');
+                    if (typeof body?.action === 'string') {
+                        apiActions.add(body.action);
+                    }
+                } catch {
+                    // Malformed JSON body — not interesting for the
+                    // action-tracking branch; the URL is still captured
+                    // above for the pathname assertion.
+                }
             }
         });
 
@@ -236,12 +254,19 @@ test.describe('flow: kickit iframe API call lands on /api.php, not /pages/api.ph
         const row0Status = page.locator('[data-testid="kickit-status-0"]');
         await expect(row0Status).toContainText('Player not found.');
 
-        // Both API actions must have round-tripped, and both URLs
-        // must target the panel-root `/api.php`.
+        // Both API actions must have round-tripped (the kickit flow
+        // is `kickit.load_servers` followed by `kickit.kick_player`
+        // per row), and every URL must target the panel-root
+        // `/api.php`. The action-set assertion is the tighter
+        // contract — without it, a regression where load_servers
+        // succeeded but kick_player went to `/pages/api.php` (or
+        // vice versa) would silently pass the "at least one POST
+        // landed on /api.php" check below.
         expect(
-            apiPostUrls.length,
-            `Expected at least 1 POST to */api.php (kickit.load_servers); saw ${apiPostUrls.length}`,
-        ).toBeGreaterThanOrEqual(1);
+            Array.from(apiActions),
+            `Expected both kickit.load_servers AND kickit.kick_player to POST to /api.php (saw: ${Array.from(apiActions).join(', ') || 'none'}). ` +
+            `If only one of the two landed, the contract regressed on the action that's missing.`,
+        ).toEqual(expect.arrayContaining(['kickit.load_servers', 'kickit.kick_player']));
 
         for (const url of apiPostUrls) {
             const path = new URL(url).pathname;