{{ message }}
Repository navigation
feat(policies): add password pwned project policy - #13702
Merged
Merged
Conversation
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>
Contributor
|
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after) |
…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>
…wned" This reverts commit e28411b.
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>
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>
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>
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>
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>
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

What does this PR do?
Adds a
password-pwnedproject policy that checks passwords against the Have I Been Pwned breach database, rejecting a breached password with a newpassword_pwnederror (HTTP 400). Users carry apasswordPwnedflag 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_DSNdefaults tonone://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,policyPasswordPwnedresponse model, SDK methodupdatePasswordPwnedPolicy. Stored asauths.passwordPwned. All update params are optional and omitted ones keep their value, likepassword-strength.Options, three in total
enabled(defaulttrue): reject a password found in any known breach when it is set or changed.sessions(defaultfalse): also check the password when a session is created, recording the outcome on the user.users(defaultfalse): block a checked sign-in withuser_password_reset_required(412) until the password is reset. Only applies whensessionsis 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\PasswordPwnedis abstract with one subclass per service, the same shape asOAuth2and its providers. A single_APP_PWNED_PASSWORDS_DSNpicks 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 useisValid()exactly as they already do forPersonalDataandPasswordHistory.hibp://localhostappwrite://SECRET@HOST[:PORT][/PATH][?tls=true]SECRETas a Bearer token (the service'sAPPWRITE_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 networknone://localhost(default)mock://localhostUser flag
passwordPwned(nullable boolean, indexed) on the User and Account models:nulluntil a password is checked,falsewhen the last check was clean,truewhen 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 tonull; an outage leaves it untouched. Filterable in users list withequal,isNull, andisNotNull. 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.passwordPwnedfrom the project, the way the personal data and history checks already read theirs, sonullon 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 invariables.php, passed indocker-compose.yml), defaulting tonone://localhostso a fresh or upgraded instance never calls out on its own.Testing without the real service: the repo
.envsetsmock://localhost, so the suite never reaches a breach service and the mock range route that used to live inapp/controllers/mock.phpis gone.Notes for reviewers
enabled: true, while the server's defaultnone://localhostanswers every check as clean, so an upgrade changes nothing on its own. Once_APP_PWNED_PASSWORDS_DSNpoints 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.Migration::$versions, following how V25 was extended onmainafter 2.2.0.Test Plan
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.PoliciesBase(console-client and custom-server suites): enable, disable, documented defaults, the sessions toggle, option preservation, empty body, invalid types, unauthenticated.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 thepasswordPwnedflag: null/false/true transitions on users get, users list, account get and account create, list filtering byequaltrue/false,isNullandisNotNullwith every returned row verified, clearing on change, forced reset still recording, outage leaving state untouched, and non-whitelisted query attributes rejected.migratefrom 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.ProjectsConsoleClientTest::testDeleteProjectWithMultiDB, which needs MongoDB and was verified failing identically with these changes reverted.Related PRs and Issues
Checklist
🤖 Generated with Claude Code