Make authorization code and password reset token consumption atomic#78
Merged
Conversation
Both endpoints had a TOCTOU where the token/code validity check and the "mark-used" write were separate statements. Two concurrent requests with the same code could both pass the !consumed check and both succeed — minting two token sets from one auth code, or resetting a password twice from one reset token. ConsumeAuthorizationCode and MarkPasswordResetTokenUsed now include the "not already used" predicate in their WHERE clause and return the row count. Handlers check rowsAffected == 1 before proceeding; 0 rows means a concurrent request won, and we refuse to issue tokens / change the password. Also reorder HandleResetPasswordPost: mark the token used BEFORE updating the password. Previously the order was reversed with an explicit "log but don't fail" on the mark-used write — meaning a transient DB error after a successful password update would leave the token valid for replay. Failing the reset when the user has to request a new token is strictly safer. Auth code replay fix matches RFC 6749 §10.5, which requires that an authorization code be usable at most once. Co-Authored-By: Claude Opus 4.7 (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
1 participant
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
Two endpoints had a TOCTOU where the "already used?" check and the mark-used write were separate statements. Concurrent requests with the same code/token could both pass the check and both proceed — minting two token sets from one auth code, or resetting a password twice from one reset token.
Fixes:
ConsumeAuthorizationCodeandMarkPasswordResetTokenUsednow includeAND consumed_at IS NULL/AND used_at IS NULLin their WHERE clauses and return the row count (:execrows).rowsAffected == 1before issuing tokens / changing the password. 0 rows ⇒ concurrent request won ⇒ refuse.HandleResetPasswordPost: mark the token used before updating the password. Previously the order was reversed with an explicit "log but don't fail" on the mark-used write, meaning a transient DB error after a successful password update would leave the token valid for replay.Auth code fix matches RFC 6749 §10.5 (codes MUST be usable at most once).
Test plan
go test ./...passesTestAuthorizationCodeCannotBeReused— exchange code twice, second attempt returns 400invalid_grantTestPasswordResetTokenCannotBeReused— reset twice with same token, second attempt returns 400; verifies DB password hash is still the first one (second attempt didn't overwrite)🤖 Generated with Claude Code