Repository navigation
feat(identity): let a legacy verifier report a password it did not check - #200
Merged
Merged
Conversation
A LegacyPasswordVerifier (or resolveUnknownIdentifier) can throw PasswordNotCheckedError when it never checked the password, such as a spent call budget or an open circuit breaker. signIn refuses the attempt exactly as it refuses a wrong password, does not count it toward the account lockout, and emits sign_in.failed with reason not_checked. Adds the optional AccountLockoutLike.releaseAttempt and LockoutStore.release, implemented by AccountLockout and MemoryLockoutStore, with matching LockoutStore contract cases. Closes #195 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
==========================================
+ Coverage 94.07% 94.19% +0.12%
==========================================
Files 148 148
Lines 11729 11834 +105
Branches 2765 2796 +31
==========================================
+ Hits 11034 11147 +113
+ Misses 232 229 -3
+ Partials 463 458 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…lock Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…residuals Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d the error default Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🎉 This PR is included in version 0.54.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.
Summary
A
LegacyPasswordVerifiercan now report that it did not check a password, separately from finding it wrong. It does this by throwing the newPasswordNotCheckedError(from@udibo/oauth2/identity/migration).IdentityService.signInrefuses that attempt the same way it refuses a wrong password, does not count it toward the account lockout, and emitssign_in.failedwith the new reasonnot_checked.Before this change, a verifier that declined to check, for example because its call budget was spent or a circuit breaker was open, looked like a wrong password. Each declined check stayed counted, so an attacker who drained the budget could lock accounts they never guessed against.
Why a typed error and not a third resolved value
verifysignature staysPromise<boolean>, andtrue/falsemean what they meant. Existing verifiers compile and behave as before."not_checked"is truthy. Any application code that callsverifier.verify()itself and tests the result withif (await verify(...))would treat it as a match and sign the person in, and the type checker would not catch that. A throw reads as a failure to every existing caller: the service before this change,verifyLegacyPassword, and an application's own code.resolveUnknownIdentifiercan throw the same error, because its return type has no room for another value.Changes
src/identity/migration.ts: addsPasswordNotCheckedError. TheverifyJSDoc now says when to throw it. Throw it only when the password never left the process. A request that was sent and then timed out, or came back with something other than a verdict, still resolvesfalseand still counts.verifyLegacyPasswordrethrows this error instead of resolvingfalse, so a dry run never reports an unchecked password as wrong. Other throws still resolvefalse.src/identity/service.ts:signInrefuses a not-checked outcome with the samenull, the same equalizing hash and the samefailedSignInFloorMsas a wrong imported password. It does not log the outcome as "verifier threw".reserveAttempt, the service takes the reservation back through the new optionalAccountLockoutLike.releaseAttempt. When that method is absent or returnsfalse, the attempt stays counted, and if it was thejustLockedreservation thelockoutevent still fires, so an unlock email is not lost. WithoutreserveAttempt,recordFailureis skipped.resolveUnknownIdentifierthrows the error, the refusal is unchanged, nothing is logged, and the event reason isnot_checked.src/identity/events.ts: adds"not_checked"toSignInFailureReason.src/identity/lockout.ts: these are optional, additive members (docs/stability.md).AccountLockoutLike.releaseAttempt?(userId, now?) => Promise<boolean>.LockoutStore.release?(userId, maxAttempts, now). It removes one count atomically, never goes below zero, and dropslockedUntilonce the count falls belowmaxAttempts. That handles overlapping not-checked attempts: the last reservation reaches the threshold and locks before any is released, and the releases then undo that lock. A lock the remaining counted attempts still reach stays in place.AccountLockout.releaseAttemptandMemoryLockoutStore.releaseimplement these members.AccountLockout.releaseAttemptresolvesfalsewhen the store has norelease.src/testing/contract/lockout-store.ts: adds optionalreleasecases, including an atomicity case where releases and increments run concurrently, plus arelease: trueoption that makes them required. The memory store runs them as required.add-login.mdcovers the store'sreleasemethod.trigger-points.mdhas updated rows.known-limitations.mddocuments the remaining window, described below.llms-full.txthas been regenerated.CHANGELOG.mdis generated by semantic-release from this commit, so it is not edited by hand.Lockout accounting after review
Review found two problems. First, a sign-in could read the account as unlocked and then reserve its attempt after overlapping unchecked attempts had locked it. That sign-in was refused but stayed counted, so the unchecked attempts alone could leave the account locked, with no
lockoutevent. Second, a lock could survive the release of thejustLockedreservation without being announced, which in log-only mode is the only signal there is. Changes made in response:"enforce", an attempt refused because the account is locked is now taken back throughreleaseAttemptwhen the lockout implements it. A refused attempt never checks a password, so not counting it never admits an extra check. During a lock earned by real failures, the count stays at or abovemaxAttemptsand the lock stands. This is a change in counting: withAccountLockoutover a store that hasrelease,failuresno longer grows while an account is locked.justLockedreservation, the service readsstatusand fires thelockoutevent if the account is still locked.releaseAttemptthrows, the service fires the event for ajustLockedreservation before rethrowing, asabandondoes.releaseleaves a record whose lock has expired alone. The next attempt then starts a fresh count, as it would after any expired lock. The contract and the suggested SQL now say so.release. One case runs releases only, and another mixes releases and increments with counts that a lost update cannot reach.instanceof, so it must come from the same copy of the package.LockoutRecord.failuresandreserveAttemptJSDoc are updated.Remaining approximations, documented in
known-limitations.md, all low severity:maxAttemptsand its conditionallockwrite makesAccountLockoutcount that attempt twice. This leaves one extra count until a successful sign-in, and it errs strict.justLockedattempt dropping its lock and the service re-reading the lock state is announced twice.A second round of review at
ba236733found nothing blocking. The follow-up commit adds a test pinning that a not-checked attempt released over a lock already announced does not announce again; the guard mutation now fails that test. It also documents the two residuals above.Testing
pnpm checkand the fullpnpm testrun pass (153 files; 4843 passed, 44 skipped).New tests:
src/identity/service.test.ts:maxAttemptsconsecutive not-checked attempts leaves the count at 0 and the account unlocked, and the next correct password signs in.maxAttemptsoverlapping not-checked attempts, the account is transiently locked and then unlocked, with nolockoutevent, and the next correct password signs in.sign_in.failedreports reasonnot_checked, andwrong_passwordis unchanged.reserveAttempt,recordFailureis never called.wrong_password.not_checked.src/identity/lockout.test.ts:releaseAttempttakes back one count, drops the lock it created, keeps a lock that still stands, leaves a missing record alone, and resolvesfalsewithout store support.src/identity/migration.test.ts:verifyLegacyPasswordrethrows the error, and other throws still resolvefalse.LockoutStorecontract has newreleasecases.Each behaviour was checked against a seeded mutation of the source, and the intended assertions failed:
justLockednot announcedunknown_identifierreleaseAttemptclaims success without store supportverifyLegacyPasswordswallows the errorEach review fix was mutation-tested the same way:
releaseOne mutant survived: keeping a zero-count record instead of deleting it. The contract allows either, so that mutant is equivalent.
Related Issues
Closes #195
🤖 Generated with Claude Code