From a7420c1579f8bfd153f9df37c9aae585707fc26a Mon Sep 17 00:00:00 2001 From: rumblefrog Date: Mon, 25 May 2026 17:15:22 -0400 Subject: [PATCH 1/2] fix(1456): collapse password-reset response branches to one envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-fix `api_auth_lost_password` threw `ApiError('not_registered', …)` on the miss branch and `ApiError('mail_failed', …)` on the SMTP- failure branch, with the success path emitting a "Check E-Mail" envelope. An unauthenticated visitor could enumerate every registered admin email on the panel by posting one address per request and reading the painted toast title back ("Check E-Mail" → registered; "Error" + "not registered" → unregistered). Post-fix every reachable branch returns the same generic "Check E-Mail" envelope ("If that email is registered, a link has been sent — check your inbox and spam folder"). DB writes + SMTP round-trip still gate on the matched branch so the panel never becomes an open mail relay AND a hostile probe doesn't get to amplify writes against `:prefix_admins.validate`. Operator-side audit-log entry on the matched-branch SMTP failure path keeps the "SMTP misconfigured" diagnostic visible without revealing per- account state to the caller. `config.enablenormallogin=0` still surfaces the `disabled` error envelope: it's an operator toggle, not a per-user signal — the value is the same for every caller. Documented the contract under "Public auth surfaces: response-shape uniformity" in AGENTS.md, with a matching Anti-patterns entry and a "Where to find what" row pointing future work at the reference shape. Sibling enumeration leaks on `api_auth_login` (the `?m=…` flag branching) are flagged for a follow-up. Caveat: the response-time differential between the matched (SMTP round-trip) and missed (immediate return) branches remains. A determined attacker can still enumerate via timing; closing that requires a background-worker queued send or a deliberate pad-the- miss approach. Both are out of scope here. The user-visible envelope-shape leak (which the reporter saw) is closed. Regression guards: - `web/tests/api/AuthTest.php::testLostPasswordResponseIsIdenticalForKnownAndUnknownEmail` asserts byte-for-byte identical wire envelopes for matched + unmatched + mail-failed branches. - `web/tests/api/__snapshots__/auth/lost_password_generic.json` locks the one canonical envelope; the obsolete `lost_password_not_registered.json` and `lost_password_mail_failed.json` snapshots are deleted. - `web/tests/e2e/specs/flows/lostpassword-toast.spec.ts` adds two chrome-side tests that drive the form submit end-to-end for known + unknown emails and assert the painted toast is identical. The file is now `test.describe.configure({ mode: 'serial' })` because the existing marquee #1403 test seeds a known `:prefix_admins.validate` token that the new tests would race under default `workers > 1`. Closes #1456. --- AGENTS.md | 119 +++++++++++++++ web/api/handlers/auth.php | 120 +++++++++++++-- web/pages/page.lostpassword.php | 15 +- web/scripts/api-contract.js | 28 ++++ web/tests/api/AuthTest.php | 139 ++++++++++++++++-- .../auth/lost_password_generic.json | 10 ++ .../auth/lost_password_mail_failed.json | 7 - .../auth/lost_password_not_registered.json | 7 - .../specs/flows/lostpassword-toast.spec.ts | 139 ++++++++++++++++++ 9 files changed, 545 insertions(+), 39 deletions(-) create mode 100644 web/tests/api/__snapshots__/auth/lost_password_generic.json delete mode 100644 web/tests/api/__snapshots__/auth/lost_password_mail_failed.json delete mode 100644 web/tests/api/__snapshots__/auth/lost_password_not_registered.json diff --git a/AGENTS.md b/AGENTS.md index af7bcdd60..50c5fd7f7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -890,6 +890,101 @@ of the diff ship together or not at all. also accepts the token via the `X-CSRF-Token` header — `sb.api.call` sets it automatically). +### Public auth surfaces: response-shape uniformity (no user enumeration) + +Any **publicly-reachable, unauthenticated** endpoint whose code path +inspects `:prefix_admins` (or any other per-account row) MUST return a +**response envelope whose shape, kind, title, and body are byte-for-byte +identical** across the matched / unmatched / mail-failed / other-server- +error branches. A user-visible differential — different error code, +different toast `kind`, different title, different body wording — is a +**user enumeration oracle**: a hostile visitor can hit the endpoint +once per address, read the envelope back, and conclude whether the +address has an admin account on this panel. That's #1456's bug class +on the password-recovery flow (`api_auth_lost_password`); the rule +generalises to every future surface that takes an identifier and looks +it up against admin state. + +The contract: + +- **All success / not-found / mail-failed / transient-error branches + return the SAME envelope.** Use a single helper function (see + `_api_auth_lost_password_generic_response()` for the reference + shape) and call it from every reachable branch. Don't `throw new + ApiError('not_registered', …)` on the miss branch — that branches + the wire shape AND the painted toast on a per-account signal. +- **Operator-side toggles ARE OK to surface.** Returning + `ApiError('disabled', …)` when `config.enablenormallogin` is off + is fine: the value is the same for every caller regardless of + what email they tried, so it doesn't reveal any per-account + signal. The matching page-handler guard must 302 away on the + same toggle so the form is never rendered, but curl-driven + callers reaching the JSON dispatcher get the structured `disabled` + envelope. +- **Body copy stays neutral.** Don't write "a reset email has been + sent to your address" (leaks: existence) or "no account was found + for that address" (leaks: non-existence). Write the conditional + ("if that email is registered, …, please check your inbox and + spam folder") — the wording is identical across every branch. +- **Audit-log discipline mirrors the response.** Log mail failures + / transient SMTP errors (Operator needs to fix SMTP). Do NOT log + the miss branch — anonymous visitors get to write to + `:prefix_log` once per request would let a hostile actor flood + the audit table at request-rate; AND the log entries would + themselves be a side-channel into "this is an unknown email" + visible to anyone who can read the audit log. +- **DB writes only happen on the matched branch.** Never UPDATE + or INSERT a row keyed on an attacker-supplied identifier when the + identifier didn't match — that's both a write amplification + vector (DoS the DB by hammering with random emails) AND would + leak via row-count / mtime / WAL traffic. The matched branch + performs the legitimate write (e.g. rolling the `validate` token); + the unmatched branch returns the generic envelope immediately. +- **SMTP only fires on the matched branch.** Never email an + attacker-supplied address when no row matched — that turns the + endpoint into an open mail relay / spam gun (any visitor can use + the panel to send mail to any address). The generic envelope + lies to the user in the missed-branch case ("If that email is + registered, a link has been sent" → it wasn't, but they don't + get told), which is the intended behavior. + +**Caveat: response-time differential remains.** The matched branch +performs a DB write + an SMTP round-trip; the unmatched branch +returns immediately. A determined attacker can still enumerate by +timing the round-trip — registered addresses take longer because +they reach the SMTP layer. Closing the timing channel requires +either a background-worker queued send (out of scope — the panel +has no worker) or a deliberate "pad the miss with a fake SMTP +delay" approach (brittle in practice: the matched-branch latency +varies with the SMTP server's mood). The response-shape uniformity +above is the load-bearing fix because it closes the trivially- +exploitable channel that motivated the issue (a hostile visitor +reading the painted toast title); the response-time leak is a +documented residual risk that requires its own follow-up. + +Reference shape: `web/api/handlers/auth.php` +(`api_auth_lost_password` + `_api_auth_lost_password_generic_response`). +Regression guards: `web/tests/api/AuthTest.php` +(`testLostPasswordResponseIsIdenticalForKnownAndUnknownEmail` is +the canonical "byte-for-byte identical" assertion) + +`web/tests/api/__snapshots__/auth/lost_password_generic.json` (the +locked wire-format) + `web/tests/e2e/specs/flows/lostpassword-toast.spec.ts` +(the chrome-side parity test — same painted toast for known + +unknown emails). + +**Sibling surfaces still in scope for follow-up:** `api_auth_login` +in `web/api/handlers/auth.php` ALSO branches its `Api::redirect()` +target on per-account signals — empty-password vs. unknown-user vs. +locked-account each redirect to a different `?m=…` flag on the +login page, which the page handler then surfaces as different toast +titles. That's a sibling enumeration oracle but with a smaller blast +radius (an attacker needs to repeatedly POST passwords to enumerate +state, and the lockout-after-5 gate inhibits sustained probing). +Tracked as a follow-up to #1456; do not silently introduce a NEW +public auth surface with the same branching shape — every new +endpoint added here goes through the response-uniformity contract +above. + ### Permissions - Web flags live in `web/configs/permissions/web.json`; `init.php` @@ -2964,6 +3059,29 @@ contacting every contributor individually. - `xajax` / `sb-callback.php` → use the JSON API. - ADOdb → use `Sbpp\Db\Database` (PDO; legacy `Database` alias still resolves via `class_alias`). +- Branching the response envelope on a per-account signal in a public + auth surface (`throw new ApiError('not_registered', …)` on the + password-reset miss branch, `throw new ApiError('mail_failed', …)` + on the SMTP-failure branch, "an email has been sent to " on + the success branch vs. "no account found" on the miss branch in + the painted toast) → use `_api_auth_lost_password_generic_response`- + shape helpers that return the SAME envelope across every reachable + branch. The pre-#1456 shape on `api_auth_lost_password` let an + unauthenticated visitor enumerate every registered admin email by + posting one address per request and reading the painted toast + title back ("Check E-Mail" → registered; "Error" + "not + registered" → unregistered). The post-fix contract is documented + under "Public auth surfaces: response-shape uniformity" in + Conventions; new public auth surfaces fall under the same rule + from day one. The orphaned snapshots + (`web/tests/api/__snapshots__/auth/lost_password_not_registered.json` + and `lost_password_mail_failed.json`) were deleted at #1456 — do + not re-add them; if a future change needs a non-generic snapshot + on this surface, the contract has regressed. Closes the + user-visible channel; the response-time differential between the + matched (SMTP round-trip) and missed (immediate return) branches + is documented as a residual risk requiring a separate (background- + worker / pad-the-miss) follow-up. - MooTools / React / a runtime bundler → vanilla JS in `web/scripts/`. - `web/scripts/sourcebans.js` (the v1.x ~1.7k-line bulk file shipping `ShowBox`, `DoLogin`, `LoadServerHost`, `selectLengthTypeReason`, …) @@ -4287,6 +4405,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//.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}/.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/.php` | +| Add or audit a publicly-reachable, unauthenticated auth surface (anything in `web/api/handlers/auth.php` or sibling registered as `requireAuth: false`) without leaking per-account state | The reference shape is `api_auth_lost_password` + `_api_auth_lost_password_generic_response` in `web/api/handlers/auth.php` (#1456). All reachable branches MUST return the same envelope; operator-side toggles (e.g. `config.enablenormallogin`) MAY surface as a per-toggle error code because the value is the same for every caller. The pre-#1456 shape branched on `not_registered` / `mail_failed` and let an unauthenticated visitor enumerate registered admin emails one request at a time by reading the painted toast back. See "Public auth surfaces: response-shape uniformity" in Conventions for the full contract (audit-log discipline, DB-write gating, SMTP gating, the documented response-time residual risk) + the matching Anti-patterns entry. Regression guards: `web/tests/api/AuthTest.php::testLostPasswordResponseIsIdenticalForKnownAndUnknownEmail` (byte-for-byte wire assertion) + `web/tests/api/__snapshots__/auth/lost_password_generic.json` (locked envelope) + `web/tests/e2e/specs/flows/lostpassword-toast.spec.ts` (chrome-side parity: same painted toast for known + unknown emails). Sibling surfaces still subject to follow-up (documented under the convention): `api_auth_login` branches its `Api::redirect()` target on per-account state via `?m=…` flags. | | 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 ``, 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 | diff --git a/web/api/handlers/auth.php b/web/api/handlers/auth.php index fb210b760..68e2a0ed9 100644 --- a/web/api/handlers/auth.php +++ b/web/api/handlers/auth.php @@ -78,9 +78,89 @@ function api_auth_login(array $params): array return Api::redirect('?' . $redirect); } +/** + * Generic response the lost-password handler returns for every reachable + * branch — registered email, unregistered email, mail-send failure. + * + * Single source of truth so a future tweak (#1456 follow-up: copy + * editing, locale support, etc.) doesn't have to be made in three + * places and silently desync the wire shape one branch uses from the + * other — which is exactly how the user-enumeration leak slips back in. + * + * The body intentionally uses "If that email is registered…" rather + * than "We sent an email to…" so the message is honest in both + * the matched and unmatched cases. Mirrors Django's password_reset + * + Rails's devise/recoverable defaults: indistinguishable response + * for present vs absent accounts is the W3C/OWASP-aligned shape + * (OWASP ASVS v4 §3.2.1; OWASP Forgot Password Cheat Sheet §"Return + * a consistent message"). + * + * @return array{message: array{title: string, body: string, kind: string}} + */ +function _api_auth_lost_password_generic_response(): array +{ + return [ + 'message' => [ + 'title' => 'Check E-Mail', + 'body' => 'If that email is registered to an admin account on this panel, ' + . 'a password reset link has been sent. ' + . 'Please check your inbox (and your spam folder).', + 'kind' => 'blue', + ], + ]; +} + +/** + * Public password-recovery entrypoint. + * + * #1456 — DO NOT reveal whether the supplied email matches a row. The + * pre-fix shape threw `ApiError('not_registered', …)` on the miss + * branch, which let an unauthenticated visitor probe the panel for + * registered email addresses one HTTP request at a time. The + * post-fix contract: + * + * - Unknown email -> generic 'Check E-Mail' envelope. + * - Known email + send ok -> generic 'Check E-Mail' envelope. + * - Known email + send err -> generic 'Check E-Mail' envelope, + * server-side audit log entry only. + * - `config.enablenormallogin` off -> `disabled` error envelope + * (an operator-side toggle, not a + * per-user signal; revealing it does + * not help an attacker enumerate). + * + * Audit-log semantics: + * + * - Unknown email: NOT logged. Logging every miss would let an + * attacker flood `:prefix_log` with arbitrary garbage and double + * as a denial-of-service against the panel's log surface. + * - Known email + send ok: not logged at the handler level. The + * follow-up reset (`page.lostpassword.php`'s `?validation=…` + * branch) logs the actual password change. + * - Known email + send err: LogType::Error so an operator can + * diagnose the SMTP misconfiguration that's silently swallowing + * reset requests (otherwise the failure would be invisible). + * + * Caveat (documented in `AGENTS.md` "Public auth surfaces …"): + * the response-shape uniformity above is the load-bearing privacy + * gate, but the response-time differential remains — the matched + * branch performs an SMTP round-trip, the missed branch does not. + * A determined attacker can still enumerate via timing. Closing + * that requires either a queued / asynchronous send (out of scope + * here — the panel has no background worker), or a deliberate + * pad-the-miss approach that's brittle in practice. Leaving the + * timing leak open is the documented trade-off; the user-visible + * envelope-shape leak (which the #1456 reporter saw) is closed. + */ function api_auth_lost_password(array $params): array { if (!Config::getBool('config.enablenormallogin')) { + // `disabled` is an operator configuration, not a per-user + // signal — same value returned for every caller — so the + // error envelope is intentional here and does NOT enable + // enumeration. The matching page-handler guard in + // `page.lostpassword.php` 302s the form away on the same + // toggle, so curl-driven callers are the only ones that + // reach this branch. throw new ApiError('disabled', 'Normal login is disabled.'); } @@ -90,8 +170,16 @@ function api_auth_lost_password(array $params): array $GLOBALS['PDO']->bind(':email', $email); $row = $GLOBALS['PDO']->single(); + // #1456: do NOT branch the response envelope on whether the email + // matched. Every reachable branch below returns the same + // _api_auth_lost_password_generic_response() shape, so a hostile + // caller can't distinguish "this address has an admin account" from + // "this address does not". The DB writes + SMTP round-trip only + // run when the row exists — never send an email to an address + // that did NOT request a reset, since that would turn the form + // into an open mail relay / spam vector. if (empty($row['aid'])) { - throw new ApiError('not_registered', 'The email address you supplied is not registered on the system'); + return _api_auth_lost_password_generic_response(); } $validation = Crypto::recoveryHash(); @@ -110,14 +198,28 @@ function api_auth_lost_password(array $params): array ]); if (!$sent) { - throw new ApiError('mail_failed', 'Error sending email.'); + // Log the failure server-side so an operator can fix the SMTP + // configuration (`Mail::send` already logs the underlying + // `Mailer::create()` / transport exception via its own + // `Log::add(LogType::Error, …)` calls, but those don't pin + // the action that triggered them). Returning the generic + // envelope means a legitimate user whose reset email failed + // to send will be told to "check spam" and never receive + // anything — operationally noisy, but the alternative is + // surfacing `mail_failed` which would let an attacker + // distinguish a registered email from an unregistered one + // simply by toggling whether SMTP is configured (or by + // submitting many requests; transient SMTP failures are + // common). + Log::add( + LogType::Error, + 'Password reset mail failed', + 'Mail::send returned false while sending the password-reset link for an admin account.' + . ' Check earlier "Mail not configured" / "Mail error" entries for the underlying cause.' + . ' The user was shown the standard "check your email" confirmation; no email was sent.' + ); + return _api_auth_lost_password_generic_response(); } - return [ - 'message' => [ - 'title' => 'Check E-Mail', - 'body' => 'Please check your email inbox (and spam) for a link which will help you reset your password.', - 'kind' => 'blue', - ], - ]; + return _api_auth_lost_password_generic_response(); } diff --git a/web/pages/page.lostpassword.php b/web/pages/page.lostpassword.php index d00b00dd4..b00b9648e 100644 --- a/web/pages/page.lostpassword.php +++ b/web/pages/page.lostpassword.php @@ -30,11 +30,16 @@ exit; } -// Issue #1102: when normal login is disabled the entire password-recovery -// flow is meaningless (a reset password can't be used to log in), and the -// reachable form would otherwise let an unauthenticated visitor probe for -// registered email addresses via the "not_registered" error. Bounce both -// the form and the recovery-link branch back to the login page. +// Issue #1102 / #1456: when normal login is disabled the entire +// password-recovery flow is meaningless — a reset password can't be +// used to log in. Bounce both the form and the recovery-link branch +// back to the login page. The companion email-enumeration leak the +// original #1102 comment referenced (the "not_registered" error +// envelope) has since been closed by #1456 — `api_auth_lost_password` +// now returns the same generic envelope regardless of whether the +// email matches an admin row — but the disable-normal-login bounce +// is still load-bearing because rendering the form on a panel where +// normal login is off would surface a non-functional UI to the user. if (!Config::getBool('config.enablenormallogin')) { header('Location: index.php?p=login'); die(); diff --git a/web/scripts/api-contract.js b/web/scripts/api-contract.js index ff5e35c82..e099be36a 100644 --- a/web/scripts/api-contract.js +++ b/web/scripts/api-contract.js @@ -67,6 +67,34 @@ * @typedef {Object} ApiAuthLoginResponse */ /** + * Public password-recovery entrypoint. #1456 — DO NOT reveal whether the + * supplied email matches a row. The pre-fix shape threw + * `ApiError('not_registered', …)` on the miss branch, which let an + * unauthenticated visitor probe the panel for registered email addresses one + * HTTP request at a time. The post-fix contract: - Unknown email -> + * generic 'Check E-Mail' envelope. - Known email + send ok -> generic 'Check + * E-Mail' envelope. - Known email + send err -> generic 'Check E-Mail' + * envelope, server-side audit log entry only. - `config.enablenormallogin` off + * -> `disabled` error envelope (an operator-side toggle, not a per-user + * signal; revealing it does not help an attacker enumerate). Audit-log + * semantics: - Unknown email: NOT logged. Logging every miss would let an + * attacker flood `:prefix_log` with arbitrary garbage and double as a + * denial-of-service against the panel's log surface. - Known email + send ok: + * not logged at the handler level. The follow-up reset + * (`page.lostpassword.php`'s `?validation=…` branch) logs the actual + * password change. - Known email + send err: LogType::Error so an operator can + * diagnose the SMTP misconfiguration that's silently swallowing reset requests + * (otherwise the failure would be invisible). Caveat (documented in + * `AGENTS.md` "Public auth surfaces …"): the response-shape uniformity above + * is the load-bearing privacy gate, but the response-time differential remains + * — the matched branch performs an SMTP round-trip, the missed branch does + * not. A determined attacker can still enumerate via timing. Closing that + * requires either a queued / asynchronous send (out of scope here — the + * panel has no background worker), or a deliberate pad-the-miss approach + * that's brittle in practice. Leaving the timing leak open is the documented + * trade-off; the user-visible envelope-shape leak (which the #1456 reporter + * saw) is closed. + * * @typedef {Object} ApiAuthLostPasswordRequest * @typedef {Object} ApiAuthLostPasswordResponse */ diff --git a/web/tests/api/AuthTest.php b/web/tests/api/AuthTest.php index 2d4654a79..b3213a913 100644 --- a/web/tests/api/AuthTest.php +++ b/web/tests/api/AuthTest.php @@ -3,26 +3,143 @@ namespace Sbpp\Tests\Api; use Sbpp\Tests\ApiTestCase; -use Sbpp\Tests\Fixture; final class AuthTest extends ApiTestCase { - public function testLostPasswordRejectsUnknownEmail(): void + /** + * #1456 — privacy fix. The pre-fix handler threw + * `ApiError('not_registered', …)` on the miss branch and let any + * unauthenticated visitor probe the panel for registered emails + * one HTTP request at a time. Post-fix every reachable branch + * (unknown email, known email + mail ok, known email + mail err) + * returns the SAME generic envelope. This test pins the miss + * branch's wire shape; the next test pins the matched-but-mail- + * failed branch; together with the snapshot they assert + * byte-for-byte equality between miss and mail-failed responses, + * which is the structural contract for #1456. + */ + public function testLostPasswordReturnsGenericResponseForUnknownEmail(): void { $env = $this->api('auth.lost_password', ['email' => 'nobody@example.test']); - $this->assertEnvelopeError($env, 'not_registered'); - $this->assertSnapshot('auth/lost_password_not_registered', $env); + $this->assertTrue($env['ok'] ?? false, 'expected ok envelope: ' . json_encode($env)); + $this->assertGenericLostPasswordEnvelope($env); + $this->assertSnapshot('auth/lost_password_generic', $env); } - public function testLostPasswordReportsMailFailureForKnownEmail(): void + /** + * Companion to {@see testLostPasswordReturnsGenericResponseForUnknownEmail} + * — without working SMTP the handler reaches the `Mail::send` + * false-branch. Pre-#1456 that translated to a `mail_failed` + * error envelope; post-fix it returns the SAME generic envelope + * as the unknown-email branch, because surfacing the mail-failed + * shape would let an attacker distinguish a registered email + * (mail attempted -> failure visible) from an unregistered one + * (mail never attempted -> immediate success), trivially + * undoing the privacy gate. + * + * We re-run the unknown-email request inline (no second API call + * — `api()` is idempotent given the same params) and assert the + * two envelopes are STRUCTURALLY identical via the same snapshot + * file. If a future refactor accidentally re-introduces a + * branch-specific code path, this assertion catches the + * drift loudly. + */ + public function testLostPasswordReturnsGenericResponseForKnownEmailEvenWhenMailFails(): void { - // Without working SMTP the handler hits the Mail::send false path, - // which translates to the structured `mail_failed` envelope. The - // mail send is best-effort by design — the wire shape of the - // failure envelope is what we are locking down here. $env = $this->api('auth.lost_password', ['email' => 'admin@example.test']); - $this->assertEnvelopeError($env, 'mail_failed'); - $this->assertSnapshot('auth/lost_password_mail_failed', $env); + $this->assertTrue($env['ok'] ?? false, 'expected ok envelope: ' . json_encode($env)); + $this->assertGenericLostPasswordEnvelope($env); + $this->assertSnapshot('auth/lost_password_generic', $env); + } + + /** + * Structural contract for #1456: the response for a known email + * is byte-for-byte indistinguishable from the response for an + * unknown email. Pinning both `===` AND the JSON-encoded form + * because an attacker observing the wire would diff the raw + * response body, not the parsed PHP array. + */ + public function testLostPasswordResponseIsIdenticalForKnownAndUnknownEmail(): void + { + $unknown = $this->api('auth.lost_password', ['email' => 'nobody@example.test']); + $known = $this->api('auth.lost_password', ['email' => 'admin@example.test']); + + $this->assertSame( + json_encode($unknown), + json_encode($known), + '#1456 — known-email and unknown-email responses must be byte-identical, ' + . 'otherwise the response shape leaks whether the address is registered', + ); + } + + /** + * The `:prefix_admins.validate` column must only be touched when + * the email actually matched a row. Probing the form for an + * unknown email must not mutate any DB state — otherwise an + * attacker could detect a hit by observing a behavior change + * after a flurry of probes. Defense in depth on top of the + * envelope-shape contract above. + */ + public function testLostPasswordDoesNotTouchAdminsTableForUnknownEmail(): void + { + $rowBefore = $this->row('admins', ['user' => 'admin']); + $this->api('auth.lost_password', ['email' => 'nobody@example.test']); + $rowAfter = $this->row('admins', ['user' => 'admin']); + + $this->assertSame( + $rowBefore['validate'] ?? null, + $rowAfter['validate'] ?? null, + 'Unknown-email probe must not touch :prefix_admins.validate on any row', + ); + } + + /** + * Conversely, a hit on a known email DOES roll the validate + * token. Without this we wouldn't detect a regression that + * accidentally short-circuited the match branch too (e.g. a + * "let's always skip the UPDATE" refactor would silently break + * the actual reset flow without any of the privacy tests above + * catching it). + */ + public function testLostPasswordRollsValidateTokenForKnownEmail(): void + { + $rowBefore = $this->row('admins', ['user' => 'admin']); + $this->api('auth.lost_password', ['email' => 'admin@example.test']); + $rowAfter = $this->row('admins', ['user' => 'admin']); + + $this->assertNotSame( + $rowBefore['validate'] ?? null, + $rowAfter['validate'] ?? null, + 'Known-email request must roll the :prefix_admins.validate token so the ' + . 'subsequent ?validation=… link can authorise the reset', + ); + $this->assertNotNull( + $rowAfter['validate'] ?? null, + 'Expected validate token to be populated for the admin row after a known-email probe', + ); + } + + /** + * Helper: assert the envelope is the documented generic shape. + * One-stop check used by both the miss-branch and matched-but- + * mail-failed-branch tests above so a tweak to the copy / kind + * lands in one place. + * + * @param array $env + */ + private function assertGenericLostPasswordEnvelope(array $env): void + { + $data = $env['data'] ?? null; + $this->assertIsArray($data, 'expected ok envelope to carry data array'); + $msg = $data['message'] ?? null; + $this->assertIsArray($msg, 'expected data.message to be an array'); + $this->assertSame('Check E-Mail', $msg['title'] ?? null); + $this->assertSame('blue', $msg['kind'] ?? null); + // Body wording is asserted via the snapshot so future copy + // edits show up as a single diff in the snapshot file rather + // than as a wall of inline string assertions here. + $this->assertIsString($msg['body'] ?? null); + $this->assertNotEmpty($msg['body']); } public function testLoginActionIsPublic(): void diff --git a/web/tests/api/__snapshots__/auth/lost_password_generic.json b/web/tests/api/__snapshots__/auth/lost_password_generic.json new file mode 100644 index 000000000..b9bf1bd76 --- /dev/null +++ b/web/tests/api/__snapshots__/auth/lost_password_generic.json @@ -0,0 +1,10 @@ +{ + "ok": true, + "data": { + "message": { + "title": "Check E-Mail", + "body": "If that email is registered to an admin account on this panel, a password reset link has been sent. Please check your inbox (and your spam folder).", + "kind": "blue" + } + } +} diff --git a/web/tests/api/__snapshots__/auth/lost_password_mail_failed.json b/web/tests/api/__snapshots__/auth/lost_password_mail_failed.json deleted file mode 100644 index 400d0d3a3..000000000 --- a/web/tests/api/__snapshots__/auth/lost_password_mail_failed.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "ok": false, - "error": { - "code": "mail_failed", - "message": "Error sending email." - } -} diff --git a/web/tests/api/__snapshots__/auth/lost_password_not_registered.json b/web/tests/api/__snapshots__/auth/lost_password_not_registered.json deleted file mode 100644 index e2d0a2122..000000000 --- a/web/tests/api/__snapshots__/auth/lost_password_not_registered.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "ok": false, - "error": { - "code": "not_registered", - "message": "The email address you supplied is not registered on the system" - } -} diff --git a/web/tests/e2e/specs/flows/lostpassword-toast.spec.ts b/web/tests/e2e/specs/flows/lostpassword-toast.spec.ts index 850d328d5..6166b887b 100644 --- a/web/tests/e2e/specs/flows/lostpassword-toast.spec.ts +++ b/web/tests/e2e/specs/flows/lostpassword-toast.spec.ts @@ -83,6 +83,20 @@ import { expect, test } from '../../fixtures/auth.ts'; import { seedLostpasswordE2e } from '../../fixtures/db.ts'; +// #1456 cross-test isolation: every test in this file touches the seeded +// admin row (`admin@example.test`) — the marquee #1403 test seeds the +// `:prefix_admins.validate` column to a known token then GETs a URL +// keyed on it; the form-POST tests below call `api_auth_lost_password` +// which UPDATEs that column to a fresh random value. With Playwright's +// default `workers > 1` the form-POST UPDATE can race the marquee +// test's seed→goto window, invalidating its token and turning a real +// regression run into a false failure. Force the file to one worker so +// the existing CI behaviour (`workers: 1`, per AGENTS.md "Playwright +// E2E specifics") is mirrored locally. The cost is ~1-2s extra wall +// time on a multi-core dev box; the benefit is deterministic test +// outcomes regardless of the harness's worker pool. +test.describe.configure({ mode: 'serial' }); + const SHORT_TOKEN = 'short'; const MISMATCH_TOKEN = '0000000000000000aaaa'; // 20 chars; the SELECT will not find a matching admin row. @@ -298,3 +312,128 @@ test.describe('flow: lostpassword toast (#1403 ShowBox → Toast::emit)', () => ).toBe(true); }); }); + +/** + * #1456 — form-POST flow privacy fix. + * + * The pre-fix shape surfaced an "Error: The email address you supplied + * is not registered on the system" toast when the user submitted an + * email that didn't match any admin row. That gave an unauthenticated + * visitor a trivial one-request-per-address oracle: type any email, + * read the toast title (Error vs Check E-Mail), conclude whether the + * address is registered on the panel. + * + * The post-fix shape returns the SAME generic "Check E-Mail" toast + * regardless of whether the email matched. These specs drive the + * `
` in `page_lostpassword.tpl` end-to-end + * (the `sb.api.call(Actions.AuthLostPassword)` round-trip + the + * chrome's `window.SBPP.showToast` paint) so a regression that + * accidentally re-introduces a branch-specific message — even one + * driven from a client-side `if (res.error.code === 'not_registered')` + * — fails the gate. + * + * The PHPUnit integration tests (`web/tests/api/AuthTest.php`) pin + * the wire-shape contract on the server side. These E2E tests pin + * the user-visible chrome behavior on the client side. Both halves + * are necessary: a future template tweak could re-introduce a + * branch-specific toast without changing the wire shape, and a + * future handler refactor could leak the wire shape without + * changing the chrome — only the pair of tests catches both axes. + */ +test.describe('flow: lostpassword form POST (#1456 user-enumeration leak)', () => { + // Logged-out-only block — page.lostpassword.php redirects + // authenticated visitors to /index.php before the form renders. + test.use({ storageState: { cookies: [], origins: [] } }); + + test('Form submission with an unknown email shows the generic "Check E-Mail" toast (NOT an error)', async ({ page }) => { + const consoleErrors: string[] = []; + page.on('pageerror', (err) => consoleErrors.push(err.message)); + + await page.goto('/index.php?p=lostpassword'); + + // The form is the static card the View renders — no GET + // parameters means the GET-validation branch is skipped and + // the page draws the form. Wait for the lucide mail icon + // chrome to settle before driving the submit so the busy- + // state assertions downstream don't race with the icon + // paint. + await expect(page.getByTestId('lostpw-email')).toBeVisible(); + + await page.getByTestId('lostpw-email').fill('definitely-not-an-admin@example.test'); + await page.getByTestId('lostpw-submit').click(); + + // The post-fix toast is the SAME envelope as the matched- + // email branch: kind=info (mapped from the API's 'blue'), + // title='Check E-Mail'. Anchor on the data-kind hook so + // a sibling chrome surface (e.g. a CSRF error toast that + // somehow snuck through) doesn't false-match. + const toast = page + .locator('.toast[data-kind="info"]') + .filter({ hasText: 'Check E-Mail' }); + await expect(toast).toBeVisible(); + + // Body wording IS asserted client-side here because the + // body-copy contract is the whole point: a "we sent you + // an email" wording would leak that the address exists, + // an "address not registered" wording would leak that it + // doesn't, and ONLY the "if that email is registered" + // wording is neutral. Spec the wording explicitly so a + // future copy edit can't silently undo the fix. + await expect(toast).toContainText(/if that email is registered/i); + + // Crucially: NO error toast paints. If the regression + // re-surfaces, the chrome would paint a kind=error toast + // with "not registered" / "Error" in it. Assert the + // absence loudly so a flaky-positive isn't possible. + const errorToast = page.locator('.toast[data-kind="error"]'); + await expect(errorToast).toHaveCount(0); + + expect( + consoleErrors, + `unexpected console errors:\n${consoleErrors.join('\n')}`, + ).toEqual([]); + }); + + test('Form submission with a known email shows the SAME generic "Check E-Mail" toast (no oracle)', async ({ page }) => { + const consoleErrors: string[] = []; + page.on('pageerror', (err) => consoleErrors.push(err.message)); + + await page.goto('/index.php?p=lostpassword'); + await expect(page.getByTestId('lostpw-email')).toBeVisible(); + + // The seeded admin row's email is admin@example.test (the + // default in `data.sql` + `Fixture::seedAdmin`). + await page.getByTestId('lostpw-email').fill('admin@example.test'); + await page.getByTestId('lostpw-submit').click(); + + // The toast HAS to look identical to the unknown-email + // case above. We assert the same selector and the same + // body wording — if the chrome started branching on + // success-vs-failure ("Your reset email has been sent" + // vs the neutral wording), the wording check would fail + // here even though the kind matches. + const toast = page + .locator('.toast[data-kind="info"]') + .filter({ hasText: 'Check E-Mail' }); + await expect(toast).toBeVisible(); + await expect(toast).toContainText(/if that email is registered/i); + + // Same loud absence assertion: no error toast paints. + // Pre-fix this branch was the only one that COULD have + // produced an error toast — the `mail_failed` envelope + // surfaced via the handler's send-failure path. Without + // a configured SMTP the e2e seed has nothing routable, + // so pre-fix this test would have caught the + // `mail_failed` -> Error toast leak. Post-fix it's a + // dual gate: catches re-introduction of either + // `not_registered` OR `mail_failed` user-visible + // branching. + const errorToast = page.locator('.toast[data-kind="error"]'); + await expect(errorToast).toHaveCount(0); + + expect( + consoleErrors, + `unexpected console errors:\n${consoleErrors.join('\n')}`, + ).toEqual([]); + }); +}); From 9579f7845c21ad19bb0d512f54164eebef3eb63f Mon Sep 17 00:00:00 2001 From: rumblefrog Date: Mon, 25 May 2026 18:13:46 -0400 Subject: [PATCH 2/2] fix(1456): apply adversarial review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S1 (AGENTS.md sibling-leak description): rewrite the api_auth_login follow-up note to accurately describe the 1-request-per-username enumeration oracle (empty-password short-circuit at line 50-52 runs BEFORE NormalAuthHandler so `attempts` is never incremented; lockout- after-5 gate provides no protection against this surface). S2 (AGENTS.md audit-log discipline): document the residual log-DoS risk on the matched-branch SMTP-failure path. An attacker who knows one registered email + catches SMTP broken can flood `:prefix_log` at request rate AND roll the legitimate user's outstanding `validate` token on every request. Closing requires a per-(IP x email) rate limiter the panel doesn't have; documented as a tracking follow-up. S3 (disabled-branch test): pin that `?config.enablenormallogin=0` returns a byte-identical envelope for matched + unmatched emails. The handler returns ApiError('disabled', ...) on this branch — same value for every caller, so the error envelope is acceptable per the response-uniformity contract — but a future regression that adds a per-account suffix would silently re-open enumeration. S4 (strengthen testLostPasswordRollsValidateTokenForKnownEmail): assert Mail::send was actually REACHED on the match branch by checking both the handler's "Password reset mail failed" log entry AND Mail::send's own "Mail not configured" entry land in `:prefix_log` after the call. Catches a regression that skips the send attempt while still rolling the token + returning the generic envelope — the real reset flow would silently never email anyone. N1 (OWASP citation fix): replace "OWASP ASVS v4 §3.2.1" (which addresses session-token confidentiality, not credential recovery) with "OWASP ASVS v4 Credential Recovery §V2.5". Also expand the framework list to include GitHub's password-recovery defaults alongside Django + Rails. N2 (neutral body copy): change "If that email is registered to an admin account on this panel" to "If an account is registered to that email address". The panel URL + form heading already advertise the surface; the response copy should not narrow the scope further. Mirrors GitHub / Django / Rails defaults. Updated the snapshot + the E2E spec's text-content assertion to match. E2E flake mitigation: the marquee #1403 happy-path test seeds `admin@example.test`'s validate column then GETs a URL keyed on it. Pre-fix the form-POST tests targeted the same row, racing it across chromium ⇄ mobile-chromium projects (test.describe.configure({ mode: 'serial' }) is within-project; CI's `workers: 1` masks the race). Lift the form-POST tests to a dedicated `lostpw-enum-known@ example.test` admin row seeded idempotently via the new `seed-lostpassword-enum-admin-e2e.php` shim + `seedLostpassword EnumAdminE2e` helper in `fixtures/db.ts`. The marquee test's pre- existing latent race (not introduced by #1456) is unaffected by this PR; CI passes deterministically and per-project local runs pass deterministically. --- AGENTS.md | 53 ++++++-- web/api/handlers/auth.php | 20 +-- web/tests/api/AuthTest.php | 117 +++++++++++++++++- .../auth/lost_password_generic.json | 2 +- web/tests/e2e/fixtures/db.ts | 61 +++++++++ .../seed-lostpassword-enum-admin-e2e.php | 108 ++++++++++++++++ .../specs/flows/lostpassword-toast.spec.ts | 71 +++++++---- 7 files changed, 383 insertions(+), 49 deletions(-) create mode 100644 web/tests/e2e/scripts/seed-lostpassword-enum-admin-e2e.php diff --git a/AGENTS.md b/AGENTS.md index 50c5fd7f7..45a1f2015 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -927,12 +927,29 @@ The contract: ("if that email is registered, …, please check your inbox and spam folder") — the wording is identical across every branch. - **Audit-log discipline mirrors the response.** Log mail failures - / transient SMTP errors (Operator needs to fix SMTP). Do NOT log - the miss branch — anonymous visitors get to write to - `:prefix_log` once per request would let a hostile actor flood - the audit table at request-rate; AND the log entries would - themselves be a side-channel into "this is an unknown email" - visible to anyone who can read the audit log. + / transient SMTP errors so an operator can diagnose them + (`Mail::send` already logs the underlying transport exception + via `LogType::Error`; the handler adds a paired entry that + pins the *action* that triggered the failure). Do NOT log the + miss branch — anonymous visitors get to write to `:prefix_log` + once per request would let a hostile actor flood the audit + table at request-rate; AND the log entries would themselves be + a side-channel into "this is an unknown email" visible to + anyone who can read the audit log. + + Residual log-DoS surface (documented, NOT closed by #1456): + the matched-branch SMTP-failure path emits a log entry per + call. An attacker who knows a single registered email AND + catches the panel with broken SMTP can hammer the endpoint + to flood `:prefix_log` (and, as a side effect, roll the + legitimate user's outstanding `validate` token on every + request, invalidating any reset link in flight). Closing that + channel cleanly requires a per-(IP × email) rate limiter the + panel doesn't currently have. The pragmatic mitigation today + is the implicit one — SMTP failures should be rare on a + healthy deployment, and the audit-log table is acceptably + sized for the access pattern — but the surface is a tracking + follow-up; do not depend on it remaining quiet under attack. - **DB writes only happen on the matched branch.** Never UPDATE or INSERT a row keyed on an attacker-supplied identifier when the identifier didn't match — that's both a write amplification @@ -977,13 +994,23 @@ in `web/api/handlers/auth.php` ALSO branches its `Api::redirect()` target on per-account signals — empty-password vs. unknown-user vs. locked-account each redirect to a different `?m=…` flag on the login page, which the page handler then surfaces as different toast -titles. That's a sibling enumeration oracle but with a smaller blast -radius (an attacker needs to repeatedly POST passwords to enumerate -state, and the lockout-after-5 gate inhibits sustained probing). -Tracked as a follow-up to #1456; do not silently introduce a NEW -public auth surface with the same branching shape — every new -endpoint added here goes through the response-uniformity contract -above. +titles. The concrete oracle: POST `{username: 'admin', password: ''}` +returns `?p=login&m=empty_pwd` (known user, empty-password +short-circuit at `api_auth_login` line 50-52 runs BEFORE +`NormalAuthHandler` so `attempts` is not incremented); POST +`{username: 'doesnotexist', password: ''}` returns +`?p=login&m=failed` (unknown-user short-circuit at line 41-43, +never touches `:prefix_admins` at all). That's a one-request-per- +username enumeration channel, no DB writes, no lockout +interaction — the `attempts` counter only gates the password- +attempt branch downstream, so the lockout-after-5 gate provides +**no** protection against this surface (the gate fires on +`NormalAuthHandler` failures, which the empty-password branch +returns before reaching). Sized similarly to the pre-#1456 +`api_auth_lost_password` leak; tracked as a follow-up to #1456. +Do not silently introduce a NEW public auth surface with the same +branching shape — every new endpoint added here goes through the +response-uniformity contract above. ### Permissions diff --git a/web/api/handlers/auth.php b/web/api/handlers/auth.php index 68e2a0ed9..c80334fb7 100644 --- a/web/api/handlers/auth.php +++ b/web/api/handlers/auth.php @@ -87,13 +87,17 @@ function api_auth_login(array $params): array * places and silently desync the wire shape one branch uses from the * other — which is exactly how the user-enumeration leak slips back in. * - * The body intentionally uses "If that email is registered…" rather - * than "We sent an email to…" so the message is honest in both - * the matched and unmatched cases. Mirrors Django's password_reset - * + Rails's devise/recoverable defaults: indistinguishable response - * for present vs absent accounts is the W3C/OWASP-aligned shape - * (OWASP ASVS v4 §3.2.1; OWASP Forgot Password Cheat Sheet §"Return - * a consistent message"). + * The body intentionally uses "If an account is registered to that + * email address…" rather than "We sent an email to…" so the message + * is honest in both the matched and unmatched cases. Mirrors + * Django's password_reset, Rails's devise/recoverable, and GitHub's + * password-recovery defaults: indistinguishable response for present + * vs absent accounts is the OWASP-aligned shape (OWASP ASVS v4 + * Credential Recovery §V2.5; OWASP Forgot Password Cheat Sheet § + * "Return a consistent message"). The wording avoids "admin account + * on this panel" — the panel URL + form heading already advertise + * the surface, but the response copy itself should not narrow the + * scope further (matches the major-framework convention). * * @return array{message: array{title: string, body: string, kind: string}} */ @@ -102,7 +106,7 @@ function _api_auth_lost_password_generic_response(): array return [ 'message' => [ 'title' => 'Check E-Mail', - 'body' => 'If that email is registered to an admin account on this panel, ' + 'body' => 'If an account is registered to that email address, ' . 'a password reset link has been sent. ' . 'Please check your inbox (and your spam folder).', 'kind' => 'blue', diff --git a/web/tests/api/AuthTest.php b/web/tests/api/AuthTest.php index b3213a913..881fc5a68 100644 --- a/web/tests/api/AuthTest.php +++ b/web/tests/api/AuthTest.php @@ -3,6 +3,7 @@ namespace Sbpp\Tests\Api; use Sbpp\Tests\ApiTestCase; +use Sbpp\Tests\Fixture; final class AuthTest extends ApiTestCase { @@ -95,17 +96,39 @@ public function testLostPasswordDoesNotTouchAdminsTableForUnknownEmail(): void /** * Conversely, a hit on a known email DOES roll the validate - * token. Without this we wouldn't detect a regression that - * accidentally short-circuited the match branch too (e.g. a - * "let's always skip the UPDATE" refactor would silently break - * the actual reset flow without any of the privacy tests above - * catching it). + * token AND reaches the Mail::send call site. Without this we + * wouldn't detect a regression that accidentally short-circuited + * the match branch too (e.g. a "let's always skip the UPDATE" + * refactor would silently break the actual reset flow without + * any of the privacy tests above catching it). + * + * The Mail::send-reached assertion is observable via the audit- + * log entry the handler emits on the SMTP-failure branch + * (`Log::add(LogType::Error, 'Password reset mail failed', …)`) + * paired with `Mail::send`'s own "Mail not configured" entry + * (the e2e fixture leaves `smtp.*` empty by default, so the + * matched branch always falls through to the failure log). + * Asserting BOTH entries land catches: + * - a regression that skips the `Mail::send` call (silent + * "no email sent" — log entry from Mail::send absent), + * - a regression that swallows the failure without logging + * (the handler's own entry absent), + * - a regression that always emits the failure log even on + * success branches (we'd see the entry on both the + * matched-mail-failed test AND the unknown-email test, + * which doesn't add a log entry). */ public function testLostPasswordRollsValidateTokenForKnownEmail(): void { $rowBefore = $this->row('admins', ['user' => 'admin']); + $logCountBefore = $this->countLogEntries('Password reset mail failed'); + $mailNotConfiguredBefore = $this->countLogEntries('Mail not configured'); + $this->api('auth.lost_password', ['email' => 'admin@example.test']); + $rowAfter = $this->row('admins', ['user' => 'admin']); + $logCountAfter = $this->countLogEntries('Password reset mail failed'); + $mailNotConfiguredAfter = $this->countLogEntries('Mail not configured'); $this->assertNotSame( $rowBefore['validate'] ?? null, @@ -117,6 +140,90 @@ public function testLostPasswordRollsValidateTokenForKnownEmail(): void $rowAfter['validate'] ?? null, 'Expected validate token to be populated for the admin row after a known-email probe', ); + $this->assertSame( + $logCountBefore + 1, + $logCountAfter, + 'Known-email request with broken SMTP must emit ONE "Password reset mail failed" ' + . 'audit-log entry so operators can diagnose the misconfiguration', + ); + $this->assertSame( + $mailNotConfiguredBefore + 1, + $mailNotConfiguredAfter, + 'Known-email request must reach Mail::send (which logs "Mail not configured" ' + . 'against the empty-SMTP fixture). Asserting this catches a regression ' + . 'that skips the actual send attempt while still rolling the token + ' + . 'returning the generic envelope — the real reset flow would silently ' + . 'never email anyone.', + ); + } + + /** + * #1456 — the `config.enablenormallogin=0` branch returns an + * `ApiError('disabled', …)` envelope. The contract documented in + * AGENTS.md "Public auth surfaces: response-shape uniformity" is + * that operator-side toggles MAY surface as a per-toggle error + * code because the value is the same for every caller — the + * envelope doesn't branch on per-account state. + * + * This test pins that contract: the disabled envelope is byte- + * identical for matched (`admin@example.test`) and unmatched + * (`nobody@example.test`) emails. A future regression that says + * "Normal login is disabled — try Steam login instead, $username" + * would diverge the two responses and break this assertion. + * + * The handler is reached because we hit the JSON API directly; + * the page-handler guard at `page.lostpassword.php:43-46` 302s + * the form away on the same toggle so browser-driven callers + * never see this surface, but curl-driven third parties do. + */ + public function testLostPasswordReturnsDisabledEnvelopeUniformlyWhenNormalLoginIsOff(): void + { + $rawPdo = Fixture::rawPdo(); + $stmt = $rawPdo->prepare(sprintf( + 'REPLACE INTO `%s_settings` (`setting`, `value`) VALUES (?, ?)', + DB_PREFIX, + )); + + try { + $stmt->execute(['config.enablenormallogin', '0']); + \Config::init($GLOBALS['PDO']); + + $unknown = $this->api('auth.lost_password', ['email' => 'nobody@example.test']); + $known = $this->api('auth.lost_password', ['email' => 'admin@example.test']); + + $this->assertFalse($unknown['ok'] ?? true, 'expected error envelope: ' . json_encode($unknown)); + $this->assertSame('disabled', $unknown['error']['code'] ?? null); + + $this->assertSame( + json_encode($unknown), + json_encode($known), + '#1456 — the disabled envelope must be byte-identical for matched ' + . 'and unmatched emails. A divergent envelope here would re-open ' + . 'enumeration on the panels that have normal login off.', + ); + } finally { + $stmt->execute(['config.enablenormallogin', '1']); + \Config::init($GLOBALS['PDO']); + } + } + + /** + * Helper: count the number of `:prefix_log` rows whose `title` + * column matches the given literal. Used by + * {@see testLostPasswordRollsValidateTokenForKnownEmail} to + * assert the matched-branch SMTP-failure path emits its + * documented audit entry. + */ + private function countLogEntries(string $title): int + { + $rawPdo = Fixture::rawPdo(); + $stmt = $rawPdo->prepare(sprintf( + 'SELECT COUNT(*) AS c FROM `%s_log` WHERE `title` = ?', + DB_PREFIX, + )); + $stmt->execute([$title]); + $row = $stmt->fetch(\PDO::FETCH_ASSOC); + return (int)($row['c'] ?? 0); } /** diff --git a/web/tests/api/__snapshots__/auth/lost_password_generic.json b/web/tests/api/__snapshots__/auth/lost_password_generic.json index b9bf1bd76..099c7873a 100644 --- a/web/tests/api/__snapshots__/auth/lost_password_generic.json +++ b/web/tests/api/__snapshots__/auth/lost_password_generic.json @@ -3,7 +3,7 @@ "data": { "message": { "title": "Check E-Mail", - "body": "If that email is registered to an admin account on this panel, a password reset link has been sent. Please check your inbox (and your spam folder).", + "body": "If an account is registered to that email address, a password reset link has been sent. Please check your inbox (and your spam folder).", "kind": "blue" } } diff --git a/web/tests/e2e/fixtures/db.ts b/web/tests/e2e/fixtures/db.ts index 6864211a3..35ebd94c0 100644 --- a/web/tests/e2e/fixtures/db.ts +++ b/web/tests/e2e/fixtures/db.ts @@ -35,6 +35,8 @@ const SEED_ANNOUNCEMENTS_INSIDE_CONTAINER = '/var/www/html/web/tests/e2e/scripts/seed-announcements-e2e.php'; const SEED_LOSTPASSWORD_INSIDE_CONTAINER = '/var/www/html/web/tests/e2e/scripts/seed-lostpassword-e2e.php'; +const SEED_LOSTPASSWORD_ENUM_ADMIN_INSIDE_CONTAINER = + '/var/www/html/web/tests/e2e/scripts/seed-lostpassword-enum-admin-e2e.php'; const SET_SETTING_INSIDE_CONTAINER = '/var/www/html/web/tests/e2e/scripts/set-setting-e2e.php'; const ORPHAN_BAN_AID_INSIDE_CONTAINER = @@ -304,6 +306,65 @@ export async function seedLostpasswordE2e(): Promise { } } +/** + * Shape of the `seedLostpasswordEnumAdminE2e` return value. + * + * Only `email` is exposed — the form-POST tests don't need a + * password / token because they never log in as the seeded user + * and the handler rolls a fresh `validate` on every match. + */ +export interface LostpasswordEnumAdminSeed { + email: string; +} + +/** + * Seed a dedicated admin row whose validate column can be freely + * rolled by the `api_auth_lost_password` handler (#1456 form-POST + * E2E tests). + * + * The seeded user lives separately from `admin@example.test` so + * the form-POST specs can drive the match branch — which UPDATEs + * `:prefix_admins.validate` — without racing the marquee #1403 + * happy-path test, which seeds `admin@example.test`'s validate + * column to a known token and `GET`s a URL keyed on it. Cross- + * project (chromium ⇄ mobile-chromium) Playwright runs are + * intrinsically parallel even with `test.describe.configure({ + * mode: 'serial' })` (serial is within-project), so the two + * tests would otherwise race on the same row. + * + * See `web/tests/e2e/scripts/seed-lostpassword-enum-admin-e2e.php` + * for the per-row contract (idempotent `INSERT IGNORE`, low- + * privilege, unguessable password). + */ +export async function seedLostpasswordEnumAdminE2e(): Promise { + const inContainer = process.env.E2E_IN_CONTAINER === '1'; + const cmd = inContainer ? 'php' : 'docker'; + const cmdArgs = inContainer + ? [SEED_LOSTPASSWORD_ENUM_ADMIN_INSIDE_CONTAINER] + : ['compose', 'exec', '-T', 'web', 'php', SEED_LOSTPASSWORD_ENUM_ADMIN_INSIDE_CONTAINER]; + + const { stdout, stderr } = await execFileP(cmd, cmdArgs, { + maxBuffer: 1 * 1024 * 1024, + cwd: inContainer ? undefined : process.cwd(), + }); + const trimmed = stdout.trim(); + if (trimmed === '') { + throw new Error(`seed-lostpassword-enum-admin-e2e.php: empty stdout\nstderr:\n${stderr}`); + } + try { + const parsed = JSON.parse(trimmed) as LostpasswordEnumAdminSeed; + if (typeof parsed.email !== 'string') { + throw new Error('missing email key'); + } + return parsed; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + throw new Error( + `seed-lostpassword-enum-admin-e2e.php: malformed stdout (${msg})\nstdout:\n${trimmed}\nstderr:\n${stderr}`, + ); + } +} + /** * Set a single `:prefix_settings` row in the e2e DB. Useful for * feature toggles that ship disabled in `data.sql` but need to be diff --git a/web/tests/e2e/scripts/seed-lostpassword-enum-admin-e2e.php b/web/tests/e2e/scripts/seed-lostpassword-enum-admin-e2e.php new file mode 100644 index 000000000..75f5f3406 --- /dev/null +++ b/web/tests/e2e/scripts/seed-lostpassword-enum-admin-e2e.php @@ -0,0 +1,108 @@ +query( + 'INSERT IGNORE INTO `:prefix_admins` (user, authid, password, gid, email, validate, extraflags, immunity) + VALUES (:user, :authid, :password, -1, :email, NULL, 0, 0)' +); +$pdo->bind(':user', 'lostpw-enum-known'); +$pdo->bind(':authid', 'STEAM_0:0:9999999'); +$pdo->bind(':password', $password); +$pdo->bind(':email', $email); +$pdo->execute(); + +fwrite(STDOUT, json_encode([ + 'email' => $email, +], JSON_THROW_ON_ERROR) . "\n"); diff --git a/web/tests/e2e/specs/flows/lostpassword-toast.spec.ts b/web/tests/e2e/specs/flows/lostpassword-toast.spec.ts index 6166b887b..5cdd12d52 100644 --- a/web/tests/e2e/specs/flows/lostpassword-toast.spec.ts +++ b/web/tests/e2e/specs/flows/lostpassword-toast.spec.ts @@ -81,20 +81,30 @@ */ import { expect, test } from '../../fixtures/auth.ts'; -import { seedLostpasswordE2e } from '../../fixtures/db.ts'; - -// #1456 cross-test isolation: every test in this file touches the seeded -// admin row (`admin@example.test`) — the marquee #1403 test seeds the -// `:prefix_admins.validate` column to a known token then GETs a URL -// keyed on it; the form-POST tests below call `api_auth_lost_password` -// which UPDATEs that column to a fresh random value. With Playwright's -// default `workers > 1` the form-POST UPDATE can race the marquee -// test's seed→goto window, invalidating its token and turning a real -// regression run into a false failure. Force the file to one worker so -// the existing CI behaviour (`workers: 1`, per AGENTS.md "Playwright -// E2E specifics") is mirrored locally. The cost is ~1-2s extra wall -// time on a multi-core dev box; the benefit is deterministic test -// outcomes regardless of the harness's worker pool. +import { seedLostpasswordE2e, seedLostpasswordEnumAdminE2e } from '../../fixtures/db.ts'; + +// #1456 cross-test isolation: the marquee #1403 test seeds the +// `admin@example.test` row's `:prefix_admins.validate` column to a +// known token then GETs a URL keyed on it. The form-POST tests at +// the tail of this file call `api_auth_lost_password` which UPDATEs +// the matched admin's `validate` to a fresh random value. Two +// safeguards keep these from racing: +// +// 1. The form-POST tests seed (and exclusively target) a DEDICATED +// admin row (`lostpw-enum-known@example.test`, see +// `seedLostpasswordEnumAdminE2e`). Hitting the same `validate` +// column as the marquee test would otherwise produce a cross- +// project flake — Playwright runs the same spec under both +// `chromium` and `mobile-chromium` IN PARALLEL by default, and +// `test.describe.configure({ mode: 'serial' })` only constrains +// within a single project's worker. The dedicated row sidesteps +// the race at the data layer rather than the scheduler layer. +// 2. We ALSO mark the file `serial` so within-project ordering is +// deterministic (CI runs `workers: 1` per AGENTS.md "Playwright +// E2E specifics"; keeping the file in serial mode mirrors that +// behaviour locally and protects against future regressions +// where two tests within this file accidentally race on a +// shared resource we hadn't anticipated). test.describe.configure({ mode: 'serial' }); const SHORT_TOKEN = 'short'; @@ -345,6 +355,19 @@ test.describe('flow: lostpassword form POST (#1456 user-enumeration leak)', () = // authenticated visitors to /index.php before the form renders. test.use({ storageState: { cookies: [], origins: [] } }); + // Use a dedicated admin row for the "known email" arm so we + // don't UPDATE `admin@example.test`'s validate column out from + // under the marquee #1403 happy-path test. Cross-project (chromium + // ⇄ mobile-chromium) Playwright runs are intrinsically parallel. + // See the top-of-file comment + `seedLostpasswordEnumAdminE2e` + // docblock for the full rationale. Idempotent shim — re-runs are + // free, so the `beforeAll` cost is one INSERT IGNORE per project. + let knownEmail: string; + test.beforeAll(async () => { + const seed = await seedLostpasswordEnumAdminE2e(); + knownEmail = seed.email; + }); + test('Form submission with an unknown email shows the generic "Check E-Mail" toast (NOT an error)', async ({ page }) => { const consoleErrors: string[] = []; page.on('pageerror', (err) => consoleErrors.push(err.message)); @@ -376,10 +399,11 @@ test.describe('flow: lostpassword form POST (#1456 user-enumeration leak)', () = // body-copy contract is the whole point: a "we sent you // an email" wording would leak that the address exists, // an "address not registered" wording would leak that it - // doesn't, and ONLY the "if that email is registered" - // wording is neutral. Spec the wording explicitly so a - // future copy edit can't silently undo the fix. - await expect(toast).toContainText(/if that email is registered/i); + // doesn't, and ONLY the conditional "if an account is + // registered" wording is neutral. Spec the wording + // explicitly so a future copy edit can't silently undo + // the fix. + await expect(toast).toContainText(/if an account is registered/i); // Crucially: NO error toast paints. If the regression // re-surfaces, the chrome would paint a kind=error toast @@ -401,9 +425,12 @@ test.describe('flow: lostpassword form POST (#1456 user-enumeration leak)', () = await page.goto('/index.php?p=lostpassword'); await expect(page.getByTestId('lostpw-email')).toBeVisible(); - // The seeded admin row's email is admin@example.test (the - // default in `data.sql` + `Fixture::seedAdmin`). - await page.getByTestId('lostpw-email').fill('admin@example.test'); + // Use the dedicated `lostpw-enum-known@example.test` row + // seeded in `beforeAll` rather than `admin@example.test` so + // we don't race the marquee #1403 happy-path test's + // `validate`-token seed (the handler UPDATEs `validate` on + // every match — see the top-of-file comment). + await page.getByTestId('lostpw-email').fill(knownEmail); await page.getByTestId('lostpw-submit').click(); // The toast HAS to look identical to the unknown-email @@ -416,7 +443,7 @@ test.describe('flow: lostpassword form POST (#1456 user-enumeration leak)', () = .locator('.toast[data-kind="info"]') .filter({ hasText: 'Check E-Mail' }); await expect(toast).toBeVisible(); - await expect(toast).toContainText(/if that email is registered/i); + await expect(toast).toContainText(/if an account is registered/i); // Same loud absence assertion: no error toast paints. // Pre-fix this branch was the only one that COULD have