diff --git a/AGENTS.md b/AGENTS.md index 882ec265b..699688a18 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3115,6 +3115,83 @@ 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…"). +- Loading `web/scripts/api.js` via dynamic injection + (`document.createElement('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 ``, 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/.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. | diff --git a/web/api/handlers/servers.php b/web/api/handlers/servers.php index 95a2d4eba..6f6a01ee1 100644 --- a/web/api/handlers/servers.php +++ b/web/api/handlers/servers.php @@ -60,8 +60,35 @@ 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. 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 084eaafcb..0613c3d9a 100644 --- a/web/scripts/api.js +++ b/web/scripts/api.js @@ -27,9 +27,57 @@ 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 + // `, 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 `