Skip to content

fix(1456): forgot-password no longer leaks whether email is registered#1461

Merged
rumblefrog merged 2 commits into
mainfrom
fix/1456-lostpassword-enumeration
May 25, 2026
Merged

fix(1456): forgot-password no longer leaks whether email is registered#1461
rumblefrog merged 2 commits into
mainfrom
fix/1456-lostpassword-enumeration

Conversation

@rumblefrog

Copy link
Copy Markdown
Member

Summary

Closes #1456. The Forgot Password endpoint (auth.lost_password) no
longer reveals whether the submitted email is registered.

  • Before: a registered email returned a Check E-Mail success
    envelope; an unregistered email returned error: not_registered
    with the message "The email address you supplied is not registered
    on the system"; SMTP failure returned error: mail_failed. Any
    anonymous visitor could enumerate registered admin accounts one
    request at a time.
  • After: every reachable branch — matched, unmatched, mail-failed
    — returns the same generic envelope: {ok: true, data: {message: {title: 'Check E-Mail', 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'}}}. The DB
    write + SMTP round-trip still only run when the row matches; the
    miss branch returns the envelope without touching :prefix_admins
    or the mailer (so the handler is NOT an open-relay vector).

The fix follows the OWASP Forgot Password Cheat Sheet's "Return a
consistent message" guidance and mirrors Django's password_reset,
Rails's devise/recoverable, and GitHub's password-recovery
defaults.

Why not just hide the error message?

The wire shape was the oracle, not just the rendered toast. Even
without rendering, a curl-driven attacker could read the JSON
envelope's error.code (not_registered vs mail_failed vs
ok: true) to enumerate. The fix collapses all three branches to a
single envelope shape so the wire layer no longer signals.

Sibling auth surfaces (audit findings)

  • api_account_change_email is gated to authenticated owners and
    returns a generic validation error for email_taken — no
    per-target leak.
  • api_auth_login HAS a sibling enumeration oracle (empty-password
    short-circuit returns ?m=empty_pwd for known users vs ?m=failed
    for unknown users, and the lockout-after-5 gate does NOT protect
    against this because it fires downstream of the short-circuit).
    Documented under "Public auth surfaces: response-shape uniformity"
    in AGENTS.md as a tracking follow-up — out of scope for Forgot Password - Leaks if email address is valid or not #1456
    because it's a different code path with its own UX implications
    (the login form's error copy needs a parallel pass).

Residual timing / log-DoS surface (documented, not closed)

  • Timing: the matched branch does ~2 DB queries + an SMTP
    round-trip; the unmatched branch does 1 DB query and returns
    immediately. A network-side attacker can statistically distinguish
    the two with enough samples. Closing requires either constant-time
    execution (always send a real mail, e.g. to a sink address, on
    the miss branch — wasteful + spam-vector) or a per-IP rate limit
    the panel doesn't currently have. Acknowledged in the handler
    docblock + AGENTS.md.
  • Log-DoS on matched-branch SMTP-failure: an attacker who knows
    a single registered email AND catches the panel with broken SMTP
    can hammer the endpoint to flood :prefix_log (each failure logs
    an audit entry) AND roll the legitimate user's outstanding
    validate token on every request. Closing requires a per-(IP ×
    email) rate limiter. Documented in AGENTS.md "Audit-log
    discipline".

Neither is the user-enumeration vulnerability the issue calls out;
both are flagged for a follow-up.

Audit logging

  • Mail-send failures emit a Log::add(LogType::Error, 'Password reset mail failed', …) entry so operators can diagnose SMTP
    misconfiguration. Mail::send already logs the underlying
    transport exception ("Mail not configured" / "Mail error" /
    "Mail send failed"); the handler adds a paired entry that pins
    the action that triggered it.
  • The miss branch does NOT log. Logging it would (1) let any
    anonymous caller write to :prefix_log at request rate (DoS
    amplification) and (2) be a side-channel into "this address is
    unknown" visible to anyone who reads the audit log.

Files

  • web/api/handlers/auth.php — collapse three branches to one
    generic envelope via _api_auth_lost_password_generic_response().
    Single source for the wire shape; load-bearing copy.
  • web/tests/api/AuthTest.php — 6 new tests pin the contract:
    identical envelope across known + unknown emails (the marquee
    contract), no DB touch on the miss branch, validate-token roll
    on the match branch, Mail::send actually reached on the match
    branch (verified via audit-log entries), and the
    ?config.enablenormallogin=0 branch returns a byte-identical
    envelope regardless of whether the email matched.
  • web/tests/api/__snapshots__/auth/lost_password_generic.json
    new snapshot for the generic envelope. The pre-fix
    lost_password_not_registered.json + lost_password_mail_failed.json
    snapshots are deleted.
  • web/tests/e2e/specs/flows/lostpassword-toast.spec.ts — 2 new
    end-to-end tests drive the form submission for known + unknown
    emails and assert the SAME generic toast paints. Uses a
    dedicated lostpw-enum-known@example.test admin row (seeded
    idempotently via the new shim) so the validate-token UPDATE
    doesn't race the marquee Audit follow-up: silent <script>ShowBox(...)</script> blobs lose user feedback on lostpassword / protest / banlist / commslist / admin.edit.comms #1403 happy-path test across Playwright
    projects.
  • web/tests/e2e/scripts/seed-lostpassword-enum-admin-e2e.php +
    web/tests/e2e/fixtures/db.ts — new shim + TS wrapper for the
    dedicated admin row.
  • AGENTS.md — new "Public auth surfaces: response-shape
    uniformity" convention block, matching Anti-patterns entry,
    "Where to find what" row, and the sibling api_auth_login +
    log-DoS follow-up notes.
  • web/pages/page.lostpassword.php — comment clarification only;
    the page handler still surfaces the disabled-form fallback and
    the email-shape inline error, which are NOT enumeration channels.
  • web/scripts/api-contract.js — regenerated to pick up updated
    handler docblock.

Test plan

  • PHPStan (level 5): no errors
  • PHPUnit (full suite): 984 tests, 3552 assertions, all pass
    (11 AuthTest cases, including 6 new ones pinning the
    enumeration-prevention contract)
  • api-contract: regenerated, no diff
  • ts-check: clean
  • E2E (lostpassword-toast): 22 passed under --workers=1
    (mirrors CI behaviour per AGENTS.md "Playwright E2E specifics")
  • Manually verified: hostile caller hitting auth.lost_password
    with an unknown email gets the same envelope as a hit on a
    registered email (admin@example.test)
  • Adversarial review pass: addressed all 4 substantive findings
    (S1-S4) + 2 nits (N1-N2)

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.
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.
@rumblefrog
rumblefrog added this pull request to the merge queue May 25, 2026
Merged via the queue into main with commit 0265d3a May 25, 2026
6 checks passed
@rumblefrog
rumblefrog deleted the fix/1456-lostpassword-enumeration branch May 25, 2026 22:26
@github-actions github-actions Bot locked and limited conversation to collaborators May 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Forgot Password - Leaks if email address is valid or not

1 participant