fix(account): stop sharing one additional_data dict across token calls - #39816
Open
ErenAta16 wants to merge 2 commits into
Open
fix(account): stop sharing one additional_data dict across token calls#39816ErenAta16 wants to merge 2 commits into
ErenAta16 wants to merge 2 commits into
Conversation
generate_reset_password_token, generate_email_register_token and generate_owner_transfer_token took additional_data as a mutable default and wrote the 6-digit code into it. Under gevent workers that one dict is shared across greenlets, and TokenManager.generate_token reads it after a Redis round-trip, so an overlapping request can store another request's code in the token payload while the caller emails its own. A caller-supplied dict was also mutated, and the default kept the last code for the process lifetime. Take None as the default and copy any caller dict before writing the code.
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
Fixes #39815.
generate_reset_password_token,generate_email_register_tokenandgenerate_owner_transfer_tokentookadditional_dataas a mutable default and then wrote the 6-digit code into it:That dict is created once at import time.
SERVER_WORKER_CLASSdefaults togevent, andgenerate_tokenreadsadditional_dataafter a Redis round-trip, so a second greenlet can overwritecodein between. The caller returns its own localcodeto be emailed while the token payload keeps the other request's — the user's emailed code then fails verification. A caller-supplied dict was mutated too, and the default retained the last code for the process lifetime.This takes
Noneas the default and copies any caller-supplied dict before writing the code. Nothing else in the signature or return value changes.Running the new tests against
main(three parametrisations each):test_additional_data_default_is_not_mutabletest_caller_dict_is_not_mutatedtest_concurrent_calls_keep_their_own_codeThe concurrency one hands control from the first caller to the second at the point
generate_tokenwould hit Redis, so it fails deterministically rather than by timing:9 failedonmain,9 passedwith the change.The rest of
tests/unit_tests/services/test_account_service.pyis unchanged at116 passed, 1 failed; that one failure (test_create_account_registration_disabled) also fails on a clean checkout, so it is not from this change.ruff checkandruff format --checkare clean on both files. I could not runtests/unit_tests/controllers/console/auth/locally — collection fails there on a stock env withAttributeError: type object 'Swagger' has no attribute 'schema_from_parameter', which looks like the pinnedflask-restxgit rev rather than anything here.Note: #39294 also changes these three signatures (adding
account_id). If that lands first this becomes a small rebase.Checklist
make lint && make type-check(backend) andcd web && pnpm exec vp staged(frontend) to appease the lint godsI ran
ruff checkandruff format --checkon the two changed files rather than the fullmake lint && make type-check, which needs the complete dependency set.From Claude Code