Close five account-enumeration and token-rotation holes in auth - #111
Merged
Conversation
Refresh rotation read RevokedAt and set it in two steps, so two callers presenting the same live token both saw null and both minted a pair - the exact concurrent-theft case rotation exists to catch, and it raised no signal at all. Rotation is now a compare-and-swap and the database picks the winner. Known residual, commented in place: the winner can insert its replacement after a loser's revocation sweep has read the table, so one pair may survive. What is guaranteed is that at most one caller succeeds and the replayed token is dead. Closing the rest needs a per-user "sessions valid from" watermark checked during token validation, which is a schema change and deliberately not done here. Register answered 409 for a known address, which is a direct enumeration oracle - and login only ran BCrypt when the account existed, so a missing one answered measurably faster. Register now answers 202 with no body either way and emails the real owner instead; login verifies against a dummy hash when the address is unknown. Measured against a running instance: register 0.296s new versus 0.286s duplicate, login 0.29s both. Fixing those exposed two more, both found by measuring rather than reading: Sending mail inline made the response wait on SMTP, which put the oracle straight back through the clock. With no mail server reachable, forgot-password answered 200 in 15ms for an unknown address and *500 in 4.2s* for a known one - enumeration by status code, and pre-existing. Register showed the same 4.1s tell. Requests now enqueue and a background service does the round trip, so no response time or status depends on whether an address exists. Register also hashed the password only on the non-duplicate path, which after the queue fix left the oracle inverted: a duplicate returned in 5ms because it skipped 250ms of BCrypt. The hash is now computed before the existence check. Two smaller repairs alongside: forgot-password left every previous code live, and AttemptCount is per row, so each request handed out another five guesses at the same six-digit space - superseded codes are now retired. And a concurrent duplicate registration raced the unique index into a 500 instead of the silent path, which is now caught on SQLSTATE 23505. ResetPasswordAsync no longer relies on a revocation helper's SaveChangesAsync to persist the new password. The API contract changes: POST /auth/register returns 202 with no body instead of 201 plus the created user. Nothing consumed the body - the mobile client already logged in immediately afterwards, and that login is still what distinguishes the two cases for whoever actually holds the password. Co-Authored-By: Claude Opus 5 (1M context) <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.
Fixes the three findings from the auth review, the two smaller notes, and two more that only showed up when I measured a running instance instead of reading the code.
1. Refresh rotation had a TOCTOU race
Two callers presenting the same live token both read
RevokedAt == null, both proceeded, both minted a pair — the exact concurrent-theft case rotation exists to catch, raising no signal at all.Now a compare-and-swap; the database picks the winner:
Known residual, commented in place: the winner can insert its replacement pair after a loser's revocation sweep has read the table, so one pair may survive. What is guaranteed is that at most one caller succeeds and the replayed token is dead. Closing the rest needs a per-user "sessions valid from" watermark checked during token validation — a schema change, deliberately not in this PR. I wrote a test asserting the stronger property first, watched it fail, and corrected the claim rather than the test.
2. Register and login leaked which emails are registered
Register answered 409 for a known address — a direct oracle. Login only ran BCrypt when the account existed, so a missing one answered measurably faster.
3 + 4. Two more, found by measuring
Fixing the above exposed problems reading the code would not have shown.
Sending mail inline put the oracle straight back through the clock. With no mail server reachable,
forgot-passwordanswered200in 15ms for an unknown address and500in 4.2s for a known one — enumeration by status code, and pre-existing. Register showed the same 4.1s tell.Requests now enqueue;
EmailDispatchBackgroundServicedoes the SMTP round trip. No response time or status depends on whether an address exists.Then register was inverted. With the queue in, a duplicate returned in 5ms versus 300ms for a new account — because it returned before hashing the password. The hash is now computed before the existence check, so both paths pay the same ~250ms.
Measured on a running instance
Same status, same zero bytes, same duration on both branches of all three endpoints.
Residual: forgot-password still differs by ~20ms (the known path does a DB write). That is far below network jitter and bounded by the 10/min per-IP limit; equalising it fully would mean faking a write for unknown addresses, which I judged worse than the ~20ms.
5 + 6. The two smaller notes
AttemptCountis per row, so every new code handed out another five guesses at the same six-digit space — "5 attempts" only ever bounded one code. Superseded codes are now retired on issue.ResetPasswordAsyncno longer relies on a revocation helper'sSaveChangesAsyncto persist the new password.Contract change
POST /auth/registerreturns 202 with no body instead of 201 plus the created user.Nothing consumed that body — the mobile client already logged in immediately afterwards, and that login is still what tells the two cases apart for whoever actually holds the password. Right password: signed in either way. Wrong one: sent to the login screen, where "forgot password" lives.
This is the one product-visible change here, and it is a one-line revert if you would rather keep the explicit "already registered" message.
Tests
New coverage: concurrent refresh lets at most one through and leaves the presented token dead; an expired token does not take the user's other sessions down; duplicate registration is byte-identical to a new one, emails the owner, and cannot change the existing password; concurrent identical registrations create exactly one account without a 500; the old reset code stops working once a new one is issued and only one stays live; login verifies a hash even for unknown addresses; and the email queue never blocks or throws at the caller, including when full.
One bug fixed in the tests themselves:
RequestResetCodeAsyncusedSentEmails.Last(...)on aConcurrentBag, which has no insertion order — it returned the oldest code, so asking twice handed back the same one. It now selects the new code by content.