Repository navigation
fix(ui): show the MFA lockout and keep the code step on a 401 - #7308
Conversation
POST /api/user/mfa/auth now answers 429 with X-Account-Lockout while wrong codes keep the source locked out, and 401 for an unknown or already used token.
A 429 from the code step used to read as a wrong code. The form now shows the lockout with the countdown the password form uses, moved to a shared hook, and keeps the MFA token so the user can retry once it ends. Fixes: shellhub-io/team#264
The MFA code step now answers a wrong code with 401, and the response interceptor treated any 401 outside /api/login as an expired session, so a typo sent the user back to the password step. A 429 without the lockout header also read as an already finished lockout, since Number(null) is 0. The tests run the real auth store through MSW. The countdown ones advance fake time one second per act() call, because the form stores the deadline after the error message renders, so its interval exists only after that commit.
|
Claude finished @geovannewashington's task in 5m 38s —— View job Code Review CompleteReviewed 7 files and posted 3 inline issues. Standards: 0 violations, 3 judgement calls. Spec: did not run. Correctness: 0. StandardsThe worst one is in SpecThe Spec axis did not run. The PR references shellhub-io/team#264, which this job can't read, so there was no issue text to review against. Additional notes
If you've addressed the feedback and want a new review, tag |
The code step now counts wrong codes apart from wrong passwords, because a correct password resets the password counter and would otherwise clear the MFA one too.
The password form parsed X-Account-Lockout with an isNaN check, so a 429 without the header read as a lockout that ended at epoch 0 and showed "Your timeout has finished" at once. Both forms now use lockoutEndFrom, which treats a missing, zero or malformed header as no countdown.
… header Cloud sets X-Account-Lockout on every response, as /api/login does, so the 200 declares it too. A valid token whose user is gone still answers 404, and a code sent while another one is being checked answers 401.
…ns it Cloud's MFA login takes a per-user in-flight guard with SetNX and deletes it when the guess is done. If a request outlives the guard's TTL, another one takes the guard, and a plain Delete from the first frees it, letting a third guess run next to the second. CompareAndDelete runs GET and DEL in one Lua script, so the owner's value decides the delete atomically. The mock was written to match mockery's output by hand, because mockery cannot load the root packages inside the dev container. Refs: shellhub-io/team#264
Summary
Companion of shellhub-io/cloud#2610, which adds the lockout and the single-use token to the MFA code step. Merge both together.
Refs shellhub-io/team#264
The OpenAPI spec of
POST /api/user/mfa/authgains 429 withX-Account-Lockout, and declares the header on 200 too. Its description says the token logs in once, wrong codes have a lockout of their own that a correct password does not clear, and a 404 means the user behind the token is gone.The interceptor change is needed because a wrong code now answers 401. Without it, a typo sends the user back to the password step.
Core's cache gains
CompareAndDelete(key, value), a Lua script that deletes a key only while it still holds the given value. Cloud's MFA guard stores a UUID per request withSetNXand releases it with this, so a request that outlives the guard's TTL cannot free the guard a later request took.type Cache interface pkg/cache/cache.go SetNX(key, value, ttl) + CompareAndDelete(key, value) redis: GET+DEL in one script, null: trueEvidence
Before: a lockout on the MFA page read as "Invalid verification code", and a 401 from the code step signed the user out.
After:
MfaLogin.test.tsxruns the real store against MSW:Login.test.tsxgains one case: a 429 without the header shows the message, no countdown and no "finished". It failed beforelockoutEndFrom.Console suite: 2670 tests pass. The build passes.
TestRedisCacheCompareAndDeleteKeepsAnotherOwnersKeyruns against Valkey: a wrong value leaves the key in place, the owner's value deletes it.Manual, dev stack: three wrong codes showed the lockout with its countdown. After it, the right code logged in without the password.
Merge Danger
Door: two-way
UI, OpenAPI and one new method on core's
Cacheinterface. No migration or wire format change. Any otherCacheimplementation has to addCompareAndDelete; the repos have only the Redis and null ones, plus the mock.Blast Radius: MFA login
SignInForm switched to the shared countdown hook and header parse. Its one change: a 429 without the header now shows no countdown, where it used to show "Your timeout has finished" at once. Merged alone, this PR is harmless: the server still answers 403 and 404, which the UI shows as before. The reverse is not true: cloud#2610 calls
CompareAndDeleteand does not build against a shellhub without it, so merge this one first or together.