Fix PasswordHistory rule blocking first-time registration - #50
Merged
Merged
Conversation
With the shipped default neev.password rules, PasswordHistory:: notReused() failed with 'Unable to verify password history.' whenever it could not resolve a user — which is precisely the first-time-signup case: there is no authenticated user, and User::findByEmail() returns null for an address that belongs to nobody yet. Result: every registration under the default config was rejected with a 422. The rule now passes vacuously when no user resolves: a nonexistent user has no password history to reuse. Authenticated password change, reset (id input), and email-resolved flows are unaffected — they all resolve a user and keep the full history check. The package's own RegistrationTest had been masking this by overriding the password rules in setUp() 'to simplify'; a new regression test now registers against the actual shipped default rules. Reported by a consuming-app developer.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codecov ReportCaution This repository is currently using the Sentry GitHub App to receive Codecov PR comments. This integration will be deprecated on July 8, 2026. Please install the Codecov GitHub App to continue receiving coverage reports on your pull requests. 📢 Thoughts on this report? Let us know! |
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
3 participants
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.
Bug (reported by a consuming-app developer — confirmed)
With the shipped default
neev.passwordrules,PasswordHistory::notReused()fails with "Unable to verify password history." whenever it cannot resolve a user. First-time registration is exactly that case: no authenticated user, andUser::findByEmail()returns null for an address that belongs to nobody yet. Every registration under the default config was rejected with a 422.Corroborating evidence that this was being masked: the package's own
RegistrationTestoverrides the password rules insetUp()"to simplify" — which is why the suite never caught it.Fix
When no user resolves, the rule now passes vacuously — a nonexistent user has no password history to reuse. All flows that do resolve a user (authenticated change-password, reset via
idinput, email-resolved flows) keep the full history check unchanged.Tests
test_passes_when_no_user_found(was asserting the buggy failure)config/neev.php, not the test-simplified ones)Note on the second report (no code change)
The same developer asked about
email_verification_method(link vs OTP): that config existed v0.3.0–v0.4.4 and was deliberately removed in v0.4.5 — verification is always a signed link now. Documented in CHANGELOG[0.4.5]and UPGRADING; a leftover key in an app's config is ignored.