feat(policies): add password pwned project policy by Meldiron · Pull Request #13702 · appwrite/appwrite · GitHub
Skip to content

feat(policies): add password pwned project policy - #13702

Merged
Meldiron merged 20 commits into
mainfrom
feat/password-pwned-policy
Sep 17, 2026
Merged

Meldiron merged 20 commits into
mainfrom
feat/password-pwned-policy

Conversation

@Meldiron

@Meldiron Meldiron commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds a password-pwned project policy that checks passwords against the Have I Been Pwned breach database, rejecting a breached password with a new password_pwned error (HTTP 400). Users carry a passwordPwned flag recording the outcome of the last check.

The policy is enabled by default per project, but the server asks nobody until it is configured. _APP_PWNED_PASSWORDS_DSN defaults to none://localhost, which reports every password as safe; point it at a breach service and projects reject breached passwords out of the box. Checking passwords on sign-in is opt-in.

  • Policy surface: PATCH /v1/project/policies/password-pwned, entries in list and get policy, policyPasswordPwned response model, SDK method updatePasswordPwnedPolicy. Stored as auths.passwordPwned. All update params are optional and omitted ones keep their value, like password-strength.

  • Options, three in total

    • enabled (default true): reject a password found in any known breach when it is set or changed.
    • sessions (default false): also check the password when a session is created, recording the outcome on the user.
    • users (default false): block a checked sign-in with user_password_reset_required (412) until the password is reset. Only applies when sessions is on.

    Lookups are server configured, so a project cannot redirect the check at its own service. When the breach service cannot be reached the request fails with general_pwned_passwords_unavailable (503) rather than silently passing a breached password.

  • Validators. Appwrite\Auth\Validator\PasswordPwned is abstract with one subclass per service, the same shape as OAuth2 and its providers. A single _APP_PWNED_PASSWORDS_DSN picks the subclass by scheme, the way storage, logging and the database connections are already configured, and an unknown scheme fails loudly rather than falling back. The request resource hands back the right subclass, so call sites use isValid() exactly as they already do for PersonalData and PasswordHistory.

    DSN Asks What leaves the server
    hibp://localhost Public Have I Been Pwned range API First five characters of the SHA-1 hash, k-anonymity. Ranges cached per prefix for an hour. No detail is read from the DSN
    appwrite://SECRET@HOST[:PORT][/PATH][?tls=true] An Appwrite Pwned service you run The whole password, authenticated with SECRET as a Bearer token (the service's APPWRITE_PWNED_SECRET). Only appropriate on a network you control or behind TLS. The service answers from its own copy of the Have I Been Pwned corpus, so nothing about the password leaves that network
    none://localhost (default) Nothing, answers in process Nothing. Reports every password as safe, so the policy protects nothing until the server is pointed at a real service. The default, and the right choice for a server that must not or cannot reach a breach service, such as an air-gapped installation
    mock://localhost Nothing, answers in process Nothing. Reports a fixed list of passwords as breached so the suite can exercise the policy. Refused on a production server, where it would report every real password as safe
  • User flag passwordPwned (nullable boolean, indexed) on the User and Account models: null until a password is checked, false when the last check was clean, true when it found a breach. Written on account and user creation, password updates, recovery, anonymous conversions, and checked sign-ins (recorded even when a forced reset blocks the sign-in). Setting a password with the policy off, clearing it, or importing a hash resets it to null; an outage leaves it untouched. Filterable in users list with equal, isNull, and isNotNull. Attribute and index are added by the V25 migration.

  • Reading the policy. The validator carries no policy of its own. Each call site reads auths.passwordPwned from the project, the way the personal data and history checks already read theirs, so null on the user attribute comes from not asking rather than from a return value.

  • Anonymous conversions: anonymous accounts setting their first password through the email or phone update endpoints now go through the same policies as account creation: strength, dictionary, personal data, pwned, and history. Existing passwords are only verified, never re-validated.

  • Config _APP_PWNED_PASSWORDS_DSN (documented in variables.php, passed in docker-compose.yml), defaulting to none://localhost so a fresh or upgraded instance never calls out on its own.

  • Testing without the real service: the repo .env sets mock://localhost, so the suite never reaches a breach service and the mock range route that used to live in app/controllers/mock.php is gone.

Notes for reviewers

  • Enabled by default reaches existing projects, but checks nothing until configured. A project that has never configured the policy inherits enabled: true, while the server's default none://localhost answers every check as clean, so an upgrade changes nothing on its own. Once _APP_PWNED_PASSWORDS_DSN points at a service, an unreachable service returns a 503 rather than passing the password. Worth a line in the release notes: set the DSN to get protection.
  • Release: the users collection gains an attribute and an index, so this must ship in a minor with the new version mapped to V25 in Migration::$versions, following how V25 was extended on main after 2.2.0.

Test Plan

  • Unit: tests/unit/Auth/Validator/PasswordPwnedTest.php, 19 cases with a named fake fetch adapter and in-memory cache: defaults, policy flags, sessions requiring an enabled policy, breach state, non-required outage, leaked, clean, padding, case, threshold, caching per prefix and endpoint, failed lookups not cached, surfacing an outage and an unexpected status, a single breach being enough, base rules, and lookups going to the configured endpoint. The lookup tests assert the k-anonymity guarantee itself, that neither the password nor its full hash nor the identifying suffix leaves the server, rather than mirroring the URL format.
  • E2E PoliciesBase (console-client and custom-server suites): enable, disable, documented defaults, the sessions toggle, option preservation, empty body, invalid types, unauthenticated.
  • E2E PoliciesPasswordPwnedIntegrationTest, 14 cases: a freshly created project rejecting a breached password with no policy change, policy off and on across users create, users password update, account create, account password update and recovery, hashed import bypass, password clearing, the sessions matrix (no check, record only, blocked until reset), anonymous email and phone conversion against every password policy, existing passwords not being re-validated, and the passwordPwned flag: null/false/true transitions on users get, users list, account get and account create, list filtering by equal true/false, isNull and isNotNull with every returned row verified, clearing on change, forced reset still recording, outage leaving state untouched, and non-whitelisted query attributes rejected.
  • Ran locally in the container against volumes migrated with migrate from the previous schema: both policy suites (92 each), the integration suite (17), Users (52), Account server, password strength, password dictionary, and the unit suite. Pint, PHPStan (level 4), and Rector check are clean.
  • One unrelated failure in that environment: ProjectsConsoleClientTest::testDeleteProjectWithMultiDB, which needs MongoDB and was verified failing identically with these changes reverted.

Related PRs and Issues

  • None

Checklist

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

🤖 Generated with Claude Code

Adds a `password-pwned` project policy, disabled by default, that checks
new passwords against the Have I Been Pwned range API using k-anonymity.
Only the first five characters of the SHA-1 hash leave the server.

When enabled, breached passwords are rejected with `password_pwned` on
account create, account password update, account recovery update, users
create, and users password update. Hashed imports are unaffected. The
check fails open when the service is unreachable.

The endpoint is configurable via `_APP_PWNED_PASSWORDS_ENDPOINT`; the dev
and CI `.env` points it at a new mock range route so automated tests never
contact the real service.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge because no unresolved new defect remains after the previous findings were manually resolved.

Summary

The PR adds a project-level breached-password policy, configurable breach-service adapters, password checks across creation and update paths, optional sign-in enforcement, persisted breach status, policy APIs, migration support, and extensive tests.

  • Adds HIBP, self-hosted Appwrite, no-op, and mock validator adapters.
  • Enforces the policy across account and user password-setting flows, including anonymous-account conversion.
  • Adds project policy endpoints and user filtering/response support for passwordPwned.
  • Adds the user attribute and index through the V25 migration.
  • Re-checks breached passwords during sign-in when configured and records the result.

Reviews (18) · Last reviewed commit: "Merge branch 'main' into feat/password-p..."

Comment thread src/Appwrite/Auth/Validator/PasswordPwned.php Outdated
Comment thread app/controllers/api/account.php Outdated
Comment thread tests/unit/Auth/Validator/PasswordPwnedTest.php Outdated
@github-actions

github-actions Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

✨ Benchmark results

Comparing main (before) → feat/password-pwned-policy (after).

Metric Before After Change
🚀 Requests/sec 213.67 215.46 ⚪ +0.8%
⏱️ Latency P50 80.21 ms 81.52 ms ⚪ +1.6%
⏱️ Latency P95 192.63 ms 188.75 ms ⚪ -2%
Per-scenario breakdown & investigation details

Metrics below reflect the current branch (after). Δ P95 compares against the base.

Scenario P50 (ms) P95 (ms) Requests RPS Δ P95 (ms)
API total 81.52 188.75 13,680 215.46 -3.88
Account 153.55 293.39 720 11.95 -16.36
TablesDB 78.73 146.87 7,440 119.79 -0.95
Storage 74.37 165.75 3,600 59.62 -1.32
Functions 113.09 231.93 1,920 32.37 -6.25

Top API waits (after)

API request Max wait (ms)
account.name.update 614.04
functions.delete 481.89
tablesdb.rows.create 442.79
functions.create 405.5
account.prefs.update 392.42

Meldiron and others added 5 commits September 15, 2026 13:16
…verage

Extends the `password-pwned` policy with options, all optional on update
and preserving the others when omitted:

- `forceReset` (default false): when true, signing in with a breached
  password fails with `user_password_reset_required` until it is reset.
- `failClosed` (default true): reject the password with the new
  `general_pwned_passwords_unavailable` (503) when the breach service
  cannot be reached; when false the check is skipped.
- `threshold` (default 1): minimum breach count before rejection.
- `endpoint` (default empty): project-level range API endpoint, falling
  back to `_APP_PWNED_PASSWORDS_ENDPOINT`, then the public service.
  Validated as a public http(s) URL like webhook URLs.

The validator is now built from the policy and registered as the
`pwnedPasswords` request resource, caching range responses per prefix
and endpoint for an hour through the cache library.

Anonymous accounts setting their first password through the email or
phone update endpoints now go through the same policies as account
creation: strength, dictionary, personal data, pwned, and history.
Existing passwords are only verified, never re-validated.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ched

Adds a nullable `passwordPwned` boolean to the users collection, with an
index, and exposes it on the User and Account models. It is null until a
password has been checked by the pwned policy, false when the last check
was clean, and true when the last check found the password in a breach.

The flag is written wherever the policy checks a password: account and
user creation, password updates, recovery, anonymous conversions, and
password sign-in. Sign-in records the outcome whether or not a reset is
forced, and an unreachable service leaves the stored value untouched.
Setting a password without a check, clearing it, or importing a hash
resets the flag to null.

Users can be listed by the flag through `Query::equal` and `isNull`.
The attribute and index are created by the V25 migration.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The project policy key, validator, and resource keep their names; only
the users collection attribute, its index, the model rule, the query
whitelist, and the local variables change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Adds a `sessions` option to the password pwned policy, off by default,
controlling whether passwords are checked when a session is created.
`forceReset` now only applies when sessions are checked, giving three
sign-in modes: no check, check and record, check and block.

Flips the policy defaults so `enabled` is true. New and existing
projects reject breached passwords out of the box, while sign-in checks
stay opt-in.

The mock range endpoint reports distinctive fixtures now, so enabling
the policy by default cannot collide with passwords other suites use.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/Appwrite/Auth/Validator/PasswordPwned.php Outdated
Meldiron and others added 2 commits September 15, 2026 14:35
Review feedback: the validator tests mirrored the URL format, HTTP
method, an internal header, and the class's own endpoint constant, so
they could never fail meaningfully and made harmless transport changes
require synchronised rewrites.

They now assert what the guarantee actually is: neither the password,
its full hash, nor the identifying suffix reaches the service, and only
the five character prefix does. Endpoint precedence is asserted with
values the test owns rather than the source constant, and the fetch
double records the URLs that left the server rather than how they were
transported.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback: with the policy now enabled by default, defaulting
`failClosed` to true meant any project that had never configured the
policy would reject password creation, updates and recovery with a 503
whenever the breach service was unreachable.

`failClosed` now defaults to false, so an outage skips the check and
leaves the recorded state untouched. Rejecting on outage stays available
by setting it to true.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/Appwrite/Auth/Validator/PasswordPwned.php Outdated
The policy is now `enabled`, `sessions` and `users`.

- `endpoint` is gone; lookups always use `_APP_PWNED_PASSWORDS_ENDPOINT`,
  so a project can no longer point the check at its own service.
- `threshold` is gone; a password found in any known breach is rejected.
- `failClosed` is gone; an unreachable breach service always surfaces
  `general_pwned_passwords_unavailable` rather than silently passing.
- `forceReset` is renamed to `users`, matching `sessions`. Its meaning is
  unchanged: a breached password blocks the sign-in until it is reset,
  and it still only applies when sessions are checked.

The outage path is now covered by unit tests only, since a project can no
longer be pointed at an unreachable endpoint from an e2e test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread app/controllers/api/account.php Outdated
The breach lookup moves out of the validator and behind an adapter, so a
server can choose who answers the question.

- `hibp` keeps the current behaviour: the public Have I Been Pwned range
  API with k-anonymity, sending only the first five characters of the
  SHA-1 hash and caching ranges per prefix.
- `appwrite` talks to an Appwrite Pwned service
  (https://github.com/appwrite-labs/pwned), which answers for a whole
  password. The password travels inside a short-lived JWT signed with
  `_APP_OPENSSL_KEY_V1`, so it is only appropriate for a service on your
  own network, and in exchange the service caches and can front a
  detector of its own choosing.

`_APP_PWNED_PASSWORDS_ADAPTER` picks the adapter and defaults to `hibp`.
`_APP_PWNED_PASSWORDS_ENDPOINT` now defaults to empty and falls back to
whichever address the chosen adapter ships with. An unknown adapter name
fails loudly at request time rather than quietly falling back.

`PasswordPwned` keeps the policy decisions and delegates the lookup, so
its unit test no longer needs an HTTP double. Each adapter has its own
test next to it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread tests/unit/Auth/Validator/PasswordPwned/AppwriteTest.php Outdated
Replaces `_APP_PWNED_PASSWORDS_ADAPTER` and `_APP_PWNED_PASSWORDS_ENDPOINT`
with a single `_APP_PWNED_PASSWORDS_DSN`, following how storage, logging
and the database connections are configured. The scheme picks the
adapter and the rest carries whatever that adapter needs.

- `hibp://localhost` reads no details, like `local://localhost` does for
  storage.
- `appwrite://SECRET@HOST[:PORT][/PATH][?tls=true]` carries the host and
  the JWT secret the service shares, so the secret is no longer borrowed
  from `_APP_OPENSSL_KEY_V1`. The path defaults to `v1/detection`.
- `mock://localhost` is new. It reports a fixed list of passwords as
  breached without leaving the process, and is refused on a production
  server because it would report every real password as safe.

The test stack now uses the mock adapter, which lets the mock range route
in `app/controllers/mock.php` go away. E2E covers the policy, the adapter
unit tests cover each protocol.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/Appwrite/Auth/Validator/PasswordPwned/Appwrite.php
Meldiron and others added 9 commits September 15, 2026 15:48
…dator

Review feedback: `Appwrite\Auth\Pwned` was a second hierarchy wrapped by a
validator whose `isValid()` nothing called, and every call site reached
for `check()` instead. That is not how validators are used here.

`PasswordPwned` is now abstract, with one subclass per service, the same
shape as `OAuth2` and its providers. The request resource hands back the
right subclass and call sites use `isValid()` like they already do for
`PersonalData` and `PasswordHistory`.

The tri-state for the `passwordPwned` user attribute did not need a
method of its own after all. A call site knows whether the policy is
enabled, so null comes from not asking rather than from a return value.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drops `isEnabled()`, `checksSessions()` and `blocksUsers()`. Each call
site now reads `auths.passwordPwned` from the project itself, the way the
personal data and history checks already read theirs, and repeats the
defaults rather than sharing them.

The validator no longer carries policy at all, so it takes no policy
argument and the request resource no longer depends on the project. What
is left is a validator that answers one question: has this password been
exposed in a breach.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drops `client()` and `unavailable()` from the abstract validator. Each
HTTP subclass builds its own client and raises
`GENERAL_PWNED_PASSWORDS_UNAVAILABLE` where the lookup fails, repeating
the timeouts rather than sharing them.

What is left on the abstract is the validator contract itself: a
description, `isValid()`, and the `isPwned()` question subclasses answer.
The mock subclass no longer inherits HTTP concerns it never used.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moves the caching into the abstract validator as a `remember()` helper,
so it is written once rather than per subclass, and gives the Appwrite
validator the cache it was missing.

Each subclass still chooses its own key, because what is worth
remembering differs: Have I Been Pwned caches a whole range of hashes per
prefix, the Appwrite service caches a single answer per password. Nothing
is stored when the lookup throws, so an outage is retried rather than
remembered.

The Appwrite key is an HMAC of the password under the secret the service
already shares, not a bare hash, so a cache dump cannot be cracked back
into user passwords.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`HIBP` and `Appwrite` declare constructors and never called the parent,
so `Password::$allowEmpty` was never initialised and every `isValid()`
call raised "Typed property must not be accessed before initialization".
`Mock` declares no constructor, so it inherited the parent's and hid the
problem from the tests that use it.

Both now forward `$allowEmpty` to the parent, the way PasswordDictionary
and PersonalData do.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
appwrite-labs/pwned#6 rewrote the Appwrite Pwned service: it answers from
its own copy of the Have I Been Pwned corpus and checks a shared secret
sent as `Authorization: Bearer <APPWRITE_PWNED_SECRET>`, in place of the
JWT it used to expect inside the body. The adapter now sends the password
as it is in the JSON body, with the DSN's user part as the Bearer token.
The answer is unchanged, `{"leaked": bool}`; every other status, a
rejected secret (401), a password the service will not take (400) or a
missing dataset (503), still surfaces as GENERAL_PWNED_PASSWORDS_UNAVAILABLE,
so a lookup that did not happen never passes as a clean password.

The unit test checks the body and headers that leave the adapter and the
service's error objects; the variable description names the new secret.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… safe

`_APP_PWNED_PASSWORDS_DSN=none://localhost` picks a validator that never
asks anyone and accepts every password, for a server that must not or
cannot reach a breach service, such as an air-gapped installation. Unlike
`mock`, it is allowed in production: it is an operator's explicit choice,
not a test fixture. Projects can still switch the policy on, but it
protects nothing until the server is pointed at a real service.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A server that has not been told which breach service to ask should ask
nobody, rather than call the public Have I Been Pwned API on its own:
`_APP_PWNED_PASSWORDS_DSN` now defaults to `none://localhost`. The
policy stays enabled by default per project, but it protects nothing
until an operator points the DSN at a service, which the variable
description now says first.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Resolves the one conflict in app/controllers/api/account.php: main made
the account-create `name` parameter nullable while this branch injects
the breach validator into the same action, so the signature now carries
both.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@Meldiron
Meldiron merged commit e5d84e0 into main Sep 17, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant