diff --git a/AGENTS.md b/AGENTS.md index a40d4bde4..033629f5f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -436,6 +436,7 @@ matching its directory. PSR-4 autoloads from `web/includes/` → | `Sbpp\Auth\Handler\SteamAuthHandler` | Steam OpenID login handler | | `Sbpp\Security\CSRF` | CSRF token helpers | | `Sbpp\Security\Crypto` | password / token crypto | +| `Sbpp\Security\PasswordGenerator` | configurable random password generator (owner defaults in `config.password.generator.*`) | | `Sbpp\Log` | audit log | | `Sbpp\Config` | settings cache | | `Sbpp\Api\Api` | JSON API dispatcher | @@ -4994,6 +4995,7 @@ the spec, target a 1920px viewport, not 1440px. | Sanitise a player display name received from an operator-controlled URL query parameter (the `?name=…` smart-default pre-fill arm on `?p=admin&c=bans§ion=add-ban` + `?p=admin&c=comms`) | `Sbpp\Util\PlayerName::sanitisePrefill(string $raw): string` (`web/includes/Util/PlayerName.php`, #1440). Single source for the strip set + UTF-8 validation + codepoint cap; both page handlers (`web/pages/admin.bans.php` + `web/pages/admin.comms.php`) call it so the contract stays byte-identical across the two surfaces. Pipeline: `trim` → `preg_replace` against `PlayerName::SANITISE_STRIP_REGEX` (ASCII controls `\x00-\x1F` + `\x7F` + C1 controls `\x80-\x9F` + soft hyphen `U+00AD` + ZWSP `U+200B` + line/paragraph separators `U+2028`/`U+2029` + bidi format/override `U+202A-U+202E` + bidi isolate `U+2066-U+2069` + BOM `U+FEFF`) → `mb_check_encoding(..., 'UTF-8')` (drop entirely on malformed input) → `mb_substr(..., 0, PlayerName::MAX_CODEPOINTS=128, 'UTF-8')` to the `varchar(128)` schema width of `:prefix_bans.name` / `:prefix_comms.name`. The bidi-control strip is the load-bearing defence against right-to-left override (`U+202E`) name-spoofing attacks where a hostile in-game name visually renders as a different string in the form's `` than what's actually stored. The codepoint-based truncation (NOT byte-based) handles 4-byte emoji without slicing mid-character. Use this helper for any future operator-controlled query-parameter that pre-fills a `varchar(128) player.name` form field; do not hand-roll a parallel strip regex (the pre-#1440 reviewer-feedback iteration was a duplicated inline `preg_replace` across both page handlers — centralisation is the contract). Regression guards: `web/tests/integration/AdminBansAddSmartDefaultTest.php` + `web/tests/integration/AdminCommsAddSmartDefaultTest.php` (`hostileNamePrefillProvider` covers every codepoint class in the strip regex + 4-byte emoji + invalid UTF-8 + 128-codepoint cap; `testNameWithoutSteamPrefillsNicknameOnly` + `testValidNameWithInvalidSteamPrefillsNicknameOnly` pin the `?name=` / `?steam=` orthogonality contract); `web/tests/e2e/specs/flows/server-player-context-menu.spec.ts` (`encodes special characters in the name parameter (#1440)` — end-to-end `encodeURIComponent` round-trip from the menu's `data-name` attribute through the form's rendered `value="…"`). | | Cache an A2S `GetInfo + GetPlayers` round-trip / add another public server-query handler | `web/includes/Servers/SourceQueryCache.php` (`Sbpp\Servers\SourceQueryCache::fetch($ip, $port, $ttl=30)` — per-`(ip, port)` on-disk cache under `SB_CACHE/srvquery/`, atomic tempfile + `rename()` writes mirroring `system.check_version`'s release cache; both success and failure cache so an unreachable server costs ONE A2S probe per ~30s window). The sibling `Sbpp\Servers\RconStatusCache` (`SB_CACHE/srvstatus/`) follows the same shape for RCON `status` round-trips — used by `api_servers_host_players` to surface per-player SteamIDs to admins (see the context-menu row above). Every public handler under `web/api/handlers/servers.php` (`api_servers_host_players` / `host_property` / `host_players_list` / `players`) goes through this — never call `new SourceQuery()` directly from a handler. The cache stamps user-agnostic data only; the handler stamps per-caller fields (`is_owner`, `can_ban`, the per-call `trunchostname`) on top. Per-tile JS debounce on the public servers page lives in `web/themes/default/page_servers.tpl` (`loadTile()` flips `tile.__sbppLoading` + the Re-query button's `disabled` attr while a probe is in flight, releases both in the success / error tails). The matching JS gate on the toggle button has been the precedent since v2.0.0; #1311 brought the refresh button onto the same shape. Tests: `web/tests/integration/SourceQueryCacheTest.php` (cache shape + coalescing + TTL + invalidation, drives `setProbeOverrideForTesting()` so the assertion is deterministic without UDP) + `testHostPlayersCoalescesRapidRepeatCallsViaCache` / `testHostPlayersNegativeCachesUnreachableServers` in `web/tests/api/ServersTest.php` (handler-shape coverage). E2E: `web/tests/e2e/specs/flows/server-refresh-debounce.spec.ts`. | | Render admin-authored Markdown to safe HTML | `web/includes/Markup/IntroRenderer.php` (`Sbpp\Markup`) | +| Add a "Generate password" button to a password field (or change how passwords are generated) | Put `data-password-generator` + `data-password-targets="[,]"` on a ` + + Generation is server-side (`Actions.AdminsGeneratePassword` → + `Sbpp\Security\PasswordGenerator`), so the charsets and the + owner-configured defaults (`config.password.generator.*`, Settings + > Main) have one source of truth. The first call of a page load + sends no options and paints the defaults the server echoes back; + later calls send the dialog's current options, so a tweak carries + over to the next field on the same page. + + Disabled targets are skipped. Filled targets get bubbling `input` + + `change` events so page-tail validators see the new value. + + Loaded globally from core/footer.tpl; a no-op on pages without a + trigger. + ============================================================ */ +(function () { + 'use strict'; + + var DIALOG_ID = 'password-generator-dialog'; + var FLAGS = ['lowercase', 'uppercase', 'digits', 'symbols', 'exclude_ambiguous']; + var SETS = ['lowercase', 'uppercase', 'digits', 'symbols']; + + /** @returns {{call: (a:string,p?:object)=>Promise}|null} */ + function api() { return /** @type {any} */ (window.sb && /** @type {any} */ (window.sb).api) || null; } + /** @returns {Record|null} */ + function actions() { return /** @type {any} */ (window).Actions || null; } + /** + * @param {Element|null} btn + * @param {boolean} busy + */ + function setBusy(btn, busy) { + if (!btn) return; + var S = /** @type {any} */ (window).SBPP; + if (S && typeof S.setBusy === 'function') S.setBusy(btn, busy); + else /** @type {HTMLButtonElement} */ (btn).disabled = busy; + } + + /** @type {string[]} */ + var targets = []; + var loaded = false; + var seq = 0; + /** @type {number|undefined} */ + var debounce; + + /** + * @param {HTMLElement} root + * @param {string} id + * @returns {HTMLInputElement} + */ + function field(root, id) { + return /** @type {HTMLInputElement} */ (root.querySelector('[data-pwgen="' + id + '"]')); + } + + /** @returns {HTMLDialogElement} */ + function ensureDialog() { + var existing = /** @type {HTMLDialogElement|null} */ (document.getElementById(DIALOG_ID)); + if (existing) return existing; + + /** + * @param {string} key + * @param {string} label + */ + function checkbox(key, label) { + return ''; + } + + var d = document.createElement('dialog'); + d.id = DIALOG_ID; + d.className = 'palette'; + d.setAttribute('aria-labelledby', DIALOG_ID + '-title'); + d.setAttribute('data-testid', 'password-generator-dialog'); + d.setAttribute('hidden', ''); + d.setAttribute('style', 'max-width:32rem;width:90vw;padding:1.25rem;border-radius:0.75rem;border:1px solid var(--border)'); + d.innerHTML = + '
' + + '

Generate password

' + + '
' + + '' + + '' + + '' + + '
' + + '
' + + '' + + '
' + + '' + + '' + + '
' + + '
' + + '
' + + 'Characters' + + '
' + + checkbox('lowercase', 'a-z') + + checkbox('uppercase', 'A-Z') + + checkbox('digits', '0-9') + + checkbox('symbols', 'Symbols') + + checkbox('exclude_ambiguous', 'Skip look-alikes') + + '
' + + '
' + + '' + + '
' + + '' + + '' + + '
' + + '
'; + document.body.appendChild(d); + wireDialog(d); + var lucide = /** @type {any} */ (window).lucide; + if (lucide && typeof lucide.createIcons === 'function') lucide.createIcons(); + return d; + } + + /** + * @param {HTMLElement} d + * @param {string} message + */ + function showError(d, message) { + var err = field(d, 'error'); + err.textContent = message; + err.hidden = message === ''; + } + + /** + * @param {HTMLElement} d + * @returns {Record} + */ + function readOptions(d) { + /** @type {Record} */ + var opts = { length: parseInt(field(d, 'length').value, 10) || 0 }; + FLAGS.forEach(function (k) { opts[k] = field(d, k).checked; }); + return opts; + } + + /** + * @param {HTMLElement} d + * @param {any} data + */ + function paint(d, data) { + var opts = data.options || {}; + var range = field(d, 'length-range'); + var num = field(d, 'length'); + [range, num].forEach(function (el) { + el.min = String(data.min_length); + el.max = String(data.max_length); + el.value = String(opts.length); + }); + FLAGS.forEach(function (k) { field(d, k).checked = !!opts[k]; }); + field(d, 'output').value = String(data.password || ''); + field(d, 'copy').setAttribute('data-copy', String(data.password || '')); + } + + /** + * @param {HTMLElement} d + */ + function generate(d) { + var a = api(), A = actions(); + var use = /** @type {HTMLButtonElement} */ (field(d, 'use')); + var regen = field(d, 'regenerate'); + if (!a || !A) { + showError(d, 'The API client is unavailable. Reload the page and try again.'); + use.disabled = true; + return; + } + + var params = {}; + if (loaded) { + params = readOptions(d); + var anySet = SETS.some(function (k) { return !!(/** @type {any} */ (params)[k]); }); + if (!anySet) { + showError(d, 'Pick at least one character set.'); + use.disabled = true; + return; + } + } + + var mine = ++seq; + use.disabled = true; + setBusy(regen, true); + a.call(A.AdminsGeneratePassword, params).then(function (r) { + if (mine !== seq) return; + setBusy(regen, false); + if (!r || r.ok === false || !r.data || !r.data.password) { + showError(d, (r && r.error && r.error.message) || 'Could not generate a password.'); + return; + } + loaded = true; + showError(d, ''); + paint(d, r.data); + use.disabled = false; + }).catch(function (err) { + if (mine !== seq) return; + setBusy(regen, false); + showError(d, String(err && err.message ? err.message : err)); + }); + } + + /** + * @param {HTMLElement} d + * @param {number} delay + */ + function scheduleGenerate(d, delay) { + window.clearTimeout(debounce); + debounce = window.setTimeout(function () { generate(d); }, delay); + } + + /** + * @param {HTMLDialogElement} d + */ + function close(d) { + window.clearTimeout(debounce); + try { d.close(); } catch (_e) { /* not opened modally */ } + d.setAttribute('hidden', ''); + } + + function fillTargets() { + var d = /** @type {HTMLDialogElement} */ (document.getElementById(DIALOG_ID)); + var value = field(d, 'output').value; + if (value === '') return; + targets.forEach(function (id) { + var el = /** @type {HTMLInputElement|null} */ (document.getElementById(id)); + if (!el || el.disabled) return; + el.value = value; + el.dispatchEvent(new Event('input', { bubbles: true })); + el.dispatchEvent(new Event('change', { bubbles: true })); + }); + } + + /** + * @param {HTMLDialogElement} d + */ + function wireDialog(d) { + var range = field(d, 'length-range'); + var num = field(d, 'length'); + + range.addEventListener('input', function () { + num.value = range.value; + scheduleGenerate(d, 150); + }); + num.addEventListener('change', function () { + var min = parseInt(num.min, 10), max = parseInt(num.max, 10); + var n = parseInt(num.value, 10); + if (isNaN(n)) n = min; + n = Math.max(min, Math.min(max, n)); + num.value = String(n); + range.value = String(n); + scheduleGenerate(d, 0); + }); + FLAGS.forEach(function (k) { + field(d, k).addEventListener('change', function () { scheduleGenerate(d, 0); }); + }); + field(d, 'regenerate').addEventListener('click', function () { generate(d); }); + field(d, 'cancel').addEventListener('click', function () { close(d); }); + d.addEventListener('cancel', function () { d.setAttribute('hidden', ''); }); + + var form = /** @type {HTMLFormElement} */ (d.querySelector('form')); + form.addEventListener('submit', function (e) { + e.preventDefault(); + if (/** @type {HTMLButtonElement} */ (field(d, 'use')).disabled) return; + fillTargets(); + close(d); + }); + } + + /** + * @param {HTMLElement} trigger + */ + function open(trigger) { + targets = (trigger.getAttribute('data-password-targets') || '') + .split(',') + .map(function (s) { return s.trim(); }) + .filter(function (s) { return s !== ''; }); + + var d = ensureDialog(); + showError(d, ''); + d.removeAttribute('hidden'); + try { d.showModal(); } + catch (_e) { d.setAttribute('open', ''); } + generate(d); + try { field(d, 'output').focus(); } catch (_e) { /* focus may throw */ } + } + + document.addEventListener('click', function (e) { + var t = /** @type {Element|null} */ (e.target); + if (!t || !t.closest) return; + var trigger = /** @type {HTMLElement|null} */ (t.closest('[data-password-generator]')); + if (!trigger || /** @type {HTMLButtonElement} */ (trigger).disabled) return; + e.preventDefault(); + open(trigger); + }); +})(); diff --git a/web/tests/api/AdminsTest.php b/web/tests/api/AdminsTest.php index 430a9cfa8..8f3bc37e2 100644 --- a/web/tests/api/AdminsTest.php +++ b/web/tests/api/AdminsTest.php @@ -412,6 +412,72 @@ public function testGeneratePasswordReturnsString(): void $this->assertSnapshot('admins/generate_password_success', $env, ['data.password']); } + public function testGeneratePasswordHonoursRequestOptions(): void + { + $this->loginAsAdmin(); + $env = $this->api('admins.generate_password', [ + 'length' => 40, + 'lowercase' => false, + 'uppercase' => false, + 'digits' => true, + 'symbols' => false, + ]); + $this->assertTrue($env['ok'], json_encode($env)); + $this->assertMatchesRegularExpression('/^[0-9]{40}$/', $env['data']['password']); + $this->assertSame(40, $env['data']['options']['length']); + $this->assertFalse($env['data']['options']['lowercase']); + $this->assertTrue($env['data']['options']['digits']); + } + + public function testGeneratePasswordClampsLength(): void + { + $this->loginAsAdmin(); + $env = $this->api('admins.generate_password', ['length' => 1]); + $this->assertTrue($env['ok'], json_encode($env)); + $this->assertSame($env['data']['min_length'], strlen($env['data']['password'])); + + $env = $this->api('admins.generate_password', ['length' => 99999]); + $this->assertSame($env['data']['max_length'], strlen($env['data']['password'])); + } + + public function testGeneratePasswordRejectsEmptyCharacterSet(): void + { + $this->loginAsAdmin(); + $env = $this->api('admins.generate_password', [ + 'lowercase' => false, + 'uppercase' => false, + 'digits' => false, + 'symbols' => false, + ]); + $this->assertEnvelopeError($env, 'validation'); + $this->assertSame('charset', $env['error']['field'] ?? null); + } + + public function testGeneratePasswordUsesConfiguredDefaults(): void + { + $this->loginAsAdmin(); + $set = static function (string $length, string $symbols): void { + Fixture::rawPdo()->prepare(sprintf( + "REPLACE INTO `%s_settings` (`setting`, `value`) VALUES + ('config.password.generator.length', ?), + ('config.password.generator.symbols', ?)", + DB_PREFIX + ))->execute([$length, $symbols]); + \Config::init($GLOBALS['PDO']); + }; + + $set('33', '0'); + try { + $env = $this->api('admins.generate_password', []); + $this->assertTrue($env['ok'], json_encode($env)); + $this->assertSame(33, strlen($env['data']['password'])); + $this->assertFalse($env['data']['options']['symbols']); + $this->assertMatchesRegularExpression('/^[A-Za-z0-9]+$/', $env['data']['password']); + } finally { + $set('20', '1'); + } + } + public function testGeneratePasswordRejectsAnonymous(): void { $env = $this->api('admins.generate_password', []); diff --git a/web/tests/api/__snapshots__/admins/generate_password_success.json b/web/tests/api/__snapshots__/admins/generate_password_success.json index f4270dd47..68bf11eda 100644 --- a/web/tests/api/__snapshots__/admins/generate_password_success.json +++ b/web/tests/api/__snapshots__/admins/generate_password_success.json @@ -1,6 +1,16 @@ { "ok": true, "data": { - "password": "<*>" + "password": "<*>", + "options": { + "length": 20, + "lowercase": true, + "uppercase": true, + "digits": true, + "symbols": true, + "exclude_ambiguous": true + }, + "min_length": 8, + "max_length": 128 } } diff --git a/web/tests/api/__snapshots__/views/youraccount_owner.json b/web/tests/api/__snapshots__/views/youraccount_owner.json index feaa2fcd4..c693c584b 100644 --- a/web/tests/api/__snapshots__/views/youraccount_owner.json +++ b/web/tests/api/__snapshots__/views/youraccount_owner.json @@ -81,6 +81,7 @@ ], "server_permissions": false, "min_pass_len": 6, - "api_tokens": [] + "api_tokens": [], + "can_generate_password": false } } 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 60a376cec..92476d7b3 100644 --- a/web/tests/e2e/specs/flows/admins-add-form.spec.ts +++ b/web/tests/e2e/specs/flows/admins-add-form.spec.ts @@ -26,9 +26,9 @@ * 1. Submitting a valid form calls `Actions.AdminsAdd`, the new * row appears on the admins list, and the operator gets a * success toast. - * 2. Clicking "Generate password" calls - * `Actions.AdminsGeneratePassword` and the password+confirm - * fields are populated with the same value. + * 2. Clicking "Generate password" opens the shared generator + * dialog (`Actions.AdminsGeneratePassword`); "Use password" + * populates the password+confirm fields with the same value. * 3. Picking "New admin group" on the server-group select reveals * the new-group name input AND the SourceMod flags input. * 4. Picking "Custom permissions" on the web-group select reveals @@ -133,7 +133,7 @@ test.describe('flow: admin admins add form (#1402 — ProcessAddAdmin zombie)', ).toEqual([]); }); - test('Generate password button → fills password + confirm', async ({ page }) => { + test('Generate password dialog → fills password + confirm', async ({ page }) => { await page.goto(ADMIN_ADMINS_ADD_ROUTE); const responsePromise = page.waitForResponse( @@ -148,16 +148,22 @@ test.describe('flow: admin admins add form (#1402 — ProcessAddAdmin zombie)', expect(typeof env.data?.password).toBe('string'); expect(env.data.password.length).toBeGreaterThan(0); - // Both fields land on the generated value. + // The shared generator dialog shows the value; "Use password" + // copies it into both fields. + const dialog = page.locator('[data-testid="password-generator-dialog"]'); + await expect(dialog).toBeVisible(); + await expect(page.locator('[data-testid="password-generator-output"]')) + .toHaveValue(env.data.password); + await page.locator('[data-testid="password-generator-use"]').click(); + await expect(dialog).toBeHidden(); + const pw1 = await page.locator('[data-testid="admin-add-password"]').inputValue(); const pw2 = await page.locator('[data-testid="admin-add-password2"]').inputValue(); expect(pw1).toBe(env.data.password); expect(pw2).toBe(env.data.password); // #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. + // stay as `password` — 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('password'); expect(await page.locator('[data-testid="admin-add-password2"]').getAttribute('type')) diff --git a/web/tests/e2e/specs/flows/password-generator.spec.ts b/web/tests/e2e/specs/flows/password-generator.spec.ts new file mode 100644 index 000000000..887066666 --- /dev/null +++ b/web/tests/e2e/specs/flows/password-generator.spec.ts @@ -0,0 +1,168 @@ +/** + * Flow spec — configurable password generator. + * + * Every "Generate password" button (`[data-password-generator]`) + * opens the shared dialog from `web/scripts/password-generator.js`, + * which calls `Actions.AdminsGeneratePassword` and fills the inputs + * named in `data-password-targets` on "Use password". + * + * What this locks in: + * 1. Per-password options (length, character sets) reach the server + * and shape the output; an empty character-set selection blocks + * "Use password" with an inline error. + * 2. The owner-configured defaults (`config.password.generator.*`) + * seed the dialog. + * 3. The generator is wired on Edit admin and Your account. + * 4. The open dialog has no critical axe violations. + * + * Nothing here submits a form: the seeded `admin/admin` password + * backs the suite's storage state and must stay unchanged. + * + * Selectors per AGENTS.md "Testability hooks": + * - `[data-testid="password-generator-dialog"]` — the dialog + * - `[data-testid="password-generator-output"]` — generated value + * - `[data-testid="password-generator-length"]` — length input + * - `[data-testid="password-generator-"]` — set checkboxes + * - `[data-testid="password-generator-error"]` — inline error + * - `[data-testid="password-generator-use"]` — fill + close + */ + +import type { Page } from '@playwright/test'; +import { expect, test } from '../../fixtures/auth.ts'; +import { expectNoCriticalA11y } from '../../fixtures/axe.ts'; +import { setSettingE2e } from '../../fixtures/db.ts'; + +const ADD_ADMIN_ROUTE = '/index.php?p=admin&c=admins§ion=add-admin'; + +const dialog = (page: Page) => page.locator('[data-testid="password-generator-dialog"]'); +const output = (page: Page) => page.locator('[data-testid="password-generator-output"]'); + +/** Waits for the next `admins.generate_password` round-trip to settle. */ +function nextGenerate(page: Page) { + return page.waitForResponse( + (r) => + r.url().includes('api.php') && + r.request().method() === 'POST' && + (r.request().postData() ?? '').includes('admins.generate_password'), + ); +} + +async function openFrom(page: Page, triggerTestId: string): Promise { + const response = nextGenerate(page); + await page.locator(`[data-testid="${triggerTestId}"]`).click(); + const env = await (await response).json(); + expect(env.ok, JSON.stringify(env)).toBe(true); + await expect(dialog(page)).toBeVisible(); + await expect(output(page)).toHaveValue(env.data.password); + return env.data.password as string; +} + +test.describe('flow: configurable password generator', () => { + test.skip(({ isMobile }) => isMobile, 'flow spec runs only on desktop chromium'); + + test('options shape the password; no character set blocks "Use password"', async ({ page }, testInfo) => { + await page.goto(ADD_ADMIN_ROUTE); + await openFrom(page, 'admin-add-generate-password'); + + await expectNoCriticalA11y(page, testInfo); + + // Digits only. + for (const set of ['lowercase', 'uppercase', 'symbols']) { + const box = page.locator(`[data-testid="password-generator-${set}"]`); + if (await box.isChecked()) { + const response = nextGenerate(page); + await box.uncheck(); + await response; + } + } + await expect(output(page)).toHaveValue(/^[0-9]+$/); + + // Exact length. + const length = page.locator('[data-testid="password-generator-length"]'); + let response = nextGenerate(page); + await length.fill('30'); + await length.press('Tab'); + await response; + await expect(output(page)).toHaveValue(/^[0-9]{30}$/); + + // Nothing selected → inline error, "Use password" disabled. + await page.locator('[data-testid="password-generator-digits"]').uncheck(); + await expect(page.locator('[data-testid="password-generator-error"]')).toBeVisible(); + await expect(page.locator('[data-testid="password-generator-use"]')).toBeDisabled(); + + // Re-enabling a set recovers. + response = nextGenerate(page); + await page.locator('[data-testid="password-generator-uppercase"]').check(); + await response; + await expect(page.locator('[data-testid="password-generator-error"]')).toBeHidden(); + await expect(output(page)).toHaveValue(/^[A-Z]{30}$/); + await expect(page.locator('[data-testid="password-generator-use"]')).toBeEnabled(); + + await page.locator('[data-testid="password-generator-use"]').click(); + await expect(dialog(page)).toBeHidden(); + await expect(page.locator('[data-testid="admin-add-password"]')).toHaveValue(/^[A-Z]{30}$/); + }); + + test.describe('configured defaults', () => { + test.afterEach(async () => { + await setSettingE2e('config.password.generator.length', '20'); + await setSettingE2e('config.password.generator.symbols', '1'); + }); + + test('dialog opens with the owner-configured defaults', async ({ page }) => { + await setSettingE2e('config.password.generator.length', '24'); + await setSettingE2e('config.password.generator.symbols', '0'); + + await page.goto(ADD_ADMIN_ROUTE); + const password = await openFrom(page, 'admin-add-generate-password'); + + expect(password).toMatch(/^[A-Za-z0-9]{24}$/); + await expect(page.locator('[data-testid="password-generator-length"]')).toHaveValue('24'); + await expect(page.locator('[data-testid="password-generator-symbols"]')).not.toBeChecked(); + }); + + test('Settings > Main saves the generator defaults', async ({ page }) => { + const route = '/index.php?p=admin&c=settings§ion=settings'; + await page.goto(route); + await page.locator('[data-testid="setting-pwgen-length"]').fill('26'); + await page.locator('[data-testid="setting-pwgen-symbols"]').uncheck(); + // The save POSTs natively, then the page bounces back after + // a short toast; anchor on the POST response, not the toast. + const saved = page.waitForResponse( + (r) => r.request().method() === 'POST' && r.url().includes('c=settings'), + ); + await page.locator('[data-testid="settings-save"]').click(); + expect((await saved).status()).toBe(200); + + await page.goto(route); + await expect(page.locator('[data-testid="setting-pwgen-length"]')).toHaveValue('26'); + await expect(page.locator('[data-testid="setting-pwgen-symbols"]')).not.toBeChecked(); + await expect(page.locator('[data-testid="setting-pwgen-lowercase"]')).toBeChecked(); + }); + }); + + test('Edit admin: fills new password + confirm', async ({ page }) => { + await page.goto('/index.php?p=admin&c=admins'); + const aid = await page + .locator('[data-testid="admin-row"][data-name="admin"]') + .first() + .getAttribute('data-id'); + expect(aid).toBeTruthy(); + + await page.goto(`/index.php?p=admin&c=admins&o=editdetails&id=${aid}`); + const password = await openFrom(page, 'edit-admin-generate-password'); + await page.locator('[data-testid="password-generator-use"]').click(); + + await expect(page.locator('[data-testid="edit-admin-password"]')).toHaveValue(password); + await expect(page.locator('[data-testid="edit-admin-password2"]')).toHaveValue(password); + }); + + test('Your account: fills new password + confirm', async ({ page }) => { + await page.goto('/index.php?p=account'); + const password = await openFrom(page, 'account-generate-password'); + await page.locator('[data-testid="password-generator-use"]').click(); + + await expect(page.locator('[data-testid="account-new-password"]')).toHaveValue(password); + await expect(page.locator('[data-testid="account-confirm-password"]')).toHaveValue(password); + }); +}); diff --git a/web/tests/unit/PasswordGeneratorTest.php b/web/tests/unit/PasswordGeneratorTest.php new file mode 100644 index 000000000..018815819 --- /dev/null +++ b/web/tests/unit/PasswordGeneratorTest.php @@ -0,0 +1,163 @@ + 32, + 'lowercase' => true, + 'uppercase' => true, + 'digits' => true, + 'symbols' => true, + 'exclude_ambiguous' => false, + ], $overrides); + } + + public function testGeneratesRequestedLength(): void + { + foreach ([PasswordGenerator::minLength(), 20, 64, PasswordGenerator::MAX_LENGTH] as $len) { + $this->assertSame($len, strlen(PasswordGenerator::generate(self::opts(['length' => $len])))); + } + } + + public function testLengthIsClampedToBounds(): void + { + $short = PasswordGenerator::generate(self::opts(['length' => 1])); + $this->assertSame(PasswordGenerator::minLength(), strlen($short)); + + $long = PasswordGenerator::generate(self::opts(['length' => 10_000])); + $this->assertSame(PasswordGenerator::MAX_LENGTH, strlen($long)); + } + + public function testMinLengthNeverDropsBelowFloorOrPanelMinimum(): void + { + $this->assertGreaterThanOrEqual(PasswordGenerator::MIN_LENGTH, PasswordGenerator::minLength()); + if (defined('MIN_PASS_LENGTH')) { + $this->assertGreaterThanOrEqual( + min(PasswordGenerator::MAX_LENGTH, (int) MIN_PASS_LENGTH), + PasswordGenerator::minLength(), + ); + } + } + + public function testEveryEnabledSetIsRepresented(): void + { + // Minimum length with all four sets: the per-set guarantee is + // what makes this pass every time, not luck. + for ($i = 0; $i < 200; $i++) { + $pw = PasswordGenerator::generate(self::opts(['length' => PasswordGenerator::minLength()])); + $this->assertMatchesRegularExpression('/[a-z]/', $pw); + $this->assertMatchesRegularExpression('/[A-Z]/', $pw); + $this->assertMatchesRegularExpression('/[0-9]/', $pw); + $this->assertMatchesRegularExpression( + '/[' . preg_quote(PasswordGenerator::SYMBOLS, '/') . ']/', + $pw, + ); + } + } + + public function testOnlyEnabledSetsAreUsed(): void + { + $digitsOnly = PasswordGenerator::generate(self::opts([ + 'lowercase' => false, 'uppercase' => false, 'symbols' => false, + ])); + $this->assertMatchesRegularExpression('/^[0-9]+$/', $digitsOnly); + + $noSymbols = PasswordGenerator::generate(self::opts(['symbols' => false, 'length' => 128])); + $this->assertMatchesRegularExpression('/^[A-Za-z0-9]+$/', $noSymbols); + } + + public function testExcludeAmbiguousDropsLookAlikes(): void + { + for ($i = 0; $i < 50; $i++) { + $pw = PasswordGenerator::generate(self::opts(['length' => 128, 'exclude_ambiguous' => true])); + $this->assertSame( + strlen($pw), + strcspn($pw, PasswordGenerator::AMBIGUOUS), + "look-alike character in '{$pw}'", + ); + } + } + + public function testSymbolsAvoidConsoleUnsafeCharacters(): void + { + foreach (['"', "'", '\\', ';', ' ', '`'] as $bad) { + $this->assertStringNotContainsString($bad, PasswordGenerator::SYMBOLS); + } + } + + public function testNoCharacterSetIsRejected(): void + { + $this->expectException(\InvalidArgumentException::class); + PasswordGenerator::generate(self::opts([ + 'lowercase' => false, 'uppercase' => false, 'digits' => false, 'symbols' => false, + ])); + } + + public function testResolveAcceptsFormStyleBooleans(): void + { + $opts = PasswordGenerator::resolve( + ['length' => '24', 'lowercase' => '1', 'uppercase' => 'false', 'digits' => 0, 'symbols' => 'on'], + self::opts(), + ); + $this->assertSame(24, $opts['length']); + $this->assertTrue($opts['lowercase']); + $this->assertFalse($opts['uppercase']); + $this->assertFalse($opts['digits']); + $this->assertTrue($opts['symbols']); + $this->assertFalse($opts['exclude_ambiguous'], 'unspecified options keep the base value'); + } + + public function testOutputIsNotDeterministic(): void + { + $seen = []; + for ($i = 0; $i < 20; $i++) { + $seen[PasswordGenerator::generate(self::opts())] = true; + } + $this->assertCount(20, $seen); + } + + /** + * Fresh installs seed from data.sql; upgrades run 813.php. Both + * must carry every key the class reads, or one install path + * silently falls back to the built-in defaults. + */ + public function testSettingsKeysAreSeededOnBothInstallPaths(): void + { + $root = dirname(__DIR__, 2); + $dataSql = (string) file_get_contents($root . '/install/includes/sql/data.sql'); + $migration = (string) file_get_contents($root . '/updater/data/813.php'); + $store = (string) file_get_contents($root . '/updater/store.json'); + + $this->assertStringContainsString('"813": "813.php"', $store); + foreach (PasswordGenerator::SETTINGS as $key) { + $this->assertMatchesRegularExpression("/\\('" . preg_quote($key, '/') . "', '[^']*'\\)/", $dataSql, "data.sql misses {$key}"); + $this->assertMatchesRegularExpression("/\\('" . preg_quote($key, '/') . "', '[^']*'\\)/", $migration, "813.php misses {$key}"); + + preg_match("/\\('" . preg_quote($key, '/') . "', '([^']*)'\\)/", $dataSql, $a); + preg_match("/\\('" . preg_quote($key, '/') . "', '([^']*)'\\)/", $migration, $b); + $this->assertSame($a[1] ?? null, $b[1] ?? null, "default for {$key} differs between data.sql and 813.php"); + } + } +} diff --git a/web/themes/default/core/footer.tpl b/web/themes/default/core/footer.tpl index e2ad84807..92ec10cc0 100644 --- a/web/themes/default/core/footer.tpl +++ b/web/themes/default/core/footer.tpl @@ -245,6 +245,12 @@ markup matches the runtime behaviour (#1402 adversarial review LOW 8). *} +{* + password-generator.js: shared "Generate password" dialog for every + `data-password-generator` trigger (Add / Edit admin, Your account, + Add / Edit server RCON). Feature-detected, a no-op elsewhere. +*} + diff --git a/web/themes/default/page_admin_admins_add.tpl b/web/themes/default/page_admin_admins_add.tpl index 58194ab8a..e44d5a8b1 100644 --- a/web/themes/default/page_admin_admins_add.tpl +++ b/web/themes/default/page_admin_admins_add.tpl @@ -121,15 +121,13 @@ data-testid="admin-add-password-toggle"> - {* #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. *} + {* Shared generator dialog (scripts/password-generator.js): + fills #password + #password2 on "Use password". *} @@ -146,7 +144,7 @@
+ onclick="var on = this.checked; ['a_serverpass', 'a_serverpass_generate'].forEach(function (id) { var el = document.getElementById(id); if (el) el.disabled = !on; });">
+
@@ -395,9 +402,9 @@ 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. + - `LoadGeneratePassword()` → now the shared generator + dialog (scripts/password-generator.js), driven by the + `data-password-generator` buttons above. - `update_server()` / `update_web()` → change handlers that reveal the conditional inputs on "Custom permissions" / "New admin group". @@ -679,46 +686,6 @@ setPasswordGroupVisible(toggle, first.type === 'password'); }); - // ---------- Generate password ---------- - document.addEventListener('click', function (e) { - var t = /** @type {Element|null} */ (e.target); - if (!t || !t.closest) return; - var btn = /** @type {HTMLElement|null} */ (t.closest('[data-action="admin-add-generate-password"]')); - if (!btn) return; - e.preventDefault(); - var a = api(), A = actions(); - if (!a || !A) return; - setBusy(btn, true); - a.call(A.AdminsGeneratePassword, {}).then(function (r) { - setBusy(btn, false); - if (!r || r.ok === false || !r.data || !r.data.password) return; - var p1 = /** @type {HTMLInputElement|null} */ (document.getElementById('password')); - var p2 = /** @type {HTMLInputElement|null} */ (document.getElementById('password2')); - if (p1) p1.value = String(r.data.password); - if (p2) p2.value = String(r.data.password); - // #1402 adversarial review MEDIUM 5: leave the input - // types as `password` (matches v1.x `LoadGeneratePassword` - // — the legacy helper never flipped .type either). - // Reset any open eye-toggle so a prior "show" click - // does not leave the freshly generated value visible. - var pwToggle = /** @type {HTMLElement|null} */ ( - document.querySelector('[data-testid="admin-add-password-toggle"]') - ); - if (pwToggle) setPasswordGroupVisible(pwToggle, false); - }).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)); - }); - }); - // ---------- Server-group / web-group conditional UI ---------- document.addEventListener('change', function (e) { var t = /** @type {Element|null} */ (e.target); diff --git a/web/themes/default/page_admin_edit_admins_details.tpl b/web/themes/default/page_admin_edit_admins_details.tpl index a349ff780..9944a4dad 100644 --- a/web/themes/default/page_admin_edit_admins_details.tpl +++ b/web/themes/default/page_admin_edit_admins_details.tpl @@ -101,8 +101,20 @@
- +
+ + {* Shared generator dialog (scripts/password-generator.js). *} + +
@@ -116,12 +128,25 @@ + onclick="var on = this.checked; ['a_serverpass', 'a_serverpass_generate'].forEach(function (id) { var el = document.getElementById(id); if (el) el.disabled = !on; });"> - +
+ + +
diff --git a/web/themes/default/page_admin_servers_add.tpl b/web/themes/default/page_admin_servers_add.tpl index 8c3f89ee7..2a3428a72 100644 --- a/web/themes/default/page_admin_servers_add.tpl +++ b/web/themes/default/page_admin_servers_add.tpl @@ -73,13 +73,25 @@
+
+ Password generator defaults +
+ + +
+
+ + + + + +
+

+ Starting options for every "Generate password" button. Admins can still adjust them per password. Length is kept between {$pwgen.min_length} and {$pwgen.max_length}. +

+
diff --git a/web/themes/default/page_youraccount.tpl b/web/themes/default/page_youraccount.tpl index 0f214dc87..acf5b9e0f 100644 --- a/web/themes/default/page_youraccount.tpl +++ b/web/themes/default/page_youraccount.tpl @@ -28,6 +28,10 @@ sb.message.* fallback so the same JS still works if this page is ever rendered under the legacy default chrome. + `data-password-generator` buttons (gated on + $can_generate_password) open the shared generator dialog from + web/scripts/password-generator.js. + Test hooks: every input + submit button carries a stable `data-testid="account-"` attribute matching the names listed in plan #1123 B20. @@ -157,12 +161,25 @@
- +
+ + {if $can_generate_password} + + {/if} +
@@ -215,11 +232,24 @@ {/if}
- +
+ + {if $can_generate_password} + + {/if} +
diff --git a/web/updater/data/813.php b/web/updater/data/813.php new file mode 100644 index 000000000..2b0f4ac73 --- /dev/null +++ b/web/updater/data/813.php @@ -0,0 +1,25 @@ +dbs` +// reads below are suppressed inline. + +// @phpstan-ignore variable.undefined +$this->dbs->query( + "INSERT IGNORE INTO `:prefix_settings` (`setting`, `value`) VALUES + ('config.password.generator.length', '20'), + ('config.password.generator.lowercase', '1'), + ('config.password.generator.uppercase', '1'), + ('config.password.generator.digits', '1'), + ('config.password.generator.symbols', '1'), + ('config.password.generator.exclude_ambiguous', '1')" +); +// @phpstan-ignore variable.undefined +$this->dbs->execute(); + +return true; diff --git a/web/updater/store.json b/web/updater/store.json index 77bbdce6c..76ba4b4af 100644 --- a/web/updater/store.json +++ b/web/updater/store.json @@ -50,5 +50,6 @@ "809": "809.php", "810": "810.php", "811": "811.php", - "812": "812.php" + "812": "812.php", + "813": "813.php" }