Mutable default argument shared across requests in the token generators (password reset path)
- Dominant language
- TypeScript
- Stars
- 156k
- Forks
- 24.6k
- Avg merge
- 22h 9m
- Merged PRs (30d)
- 610
Description
### Self Checks
- [x] I have searched the existing issues and found none matching this.
- [x] I am using the latest `main`.
### Dify version
`main` (f9966c2)
### Cloud or Self Hosted
Self Hosted (Docker), also applies to Cloud
### Steps to reproduce
The three token generators in `api/services/account_service.py` take a **mutable default argument** and then write into it:
```python
@classmethod
def generate_reset_password_token(
cls,
email: str,
account: Account | None = None,
code: str | None = None,
additional_data: dict[str, Any] = {}, # <- created once, at def time
):
if not code:
code = "".join([str(secrets.randbelow(exclusive_upper_bound=10)) for _ in range(6)])
additional_data["code"] = code # <- mutates that shared dict
token = TokenManager.generate_token(
account=account, email=email, token_type="reset_password", additional_data=additional_data
)
return code, token
```
Same shape in `generate_email_register_token` (L909) and `generate_owner_transfer_token` (L936).
The default `{}` is **one object shared by every call that omits the argument**, and all three in-tree callers omit it:
- `account_service.py:707` — `send_reset_password_email`
- `account_service.py:740` — `send_email_register_email`
- `account_service.py:840` — `send_owner_transfer_email`
### Why this is not just a style problem here
There is a yield point between the write and the read. `additional_data` is written in `account_service`, but only read later inside `TokenManager.generate_token` (`api/libs/helper.py:519`, `token_data.update(additional_data)`). In between, when an `account` is supplied — which `send_reset_password_email` does — `generate_token` runs:
```python
if account_id:
old_token = cls._get_current_token_for_account(account_id, token_type) # Redis round-trip
if old_token:
cls.revoke_token(old_token, token_type) # Redis round-trip
```
The default worker is gevent (`SERVER_WORKER_CLASS: geventwebsocket.gunicorn.workers.GeventWebSocketWorker` in `docker/docker-compose.yaml`, `--worker-class` in `api/docker/entrypoint.sh`), so those Redis calls yield to other greenlets **in the same process, sharing the same dict**. Two password-reset requests can interleave:
| | greenlet A | greenlet B | shared default dict |
|---|---|---|---|
| 1 | `additional_data["code"] = "111111"` | | `{"code": "111111"}` |
| 2 | Redis in `generate_token` → yields | | |
| 3 | | `additional_data["code"] = "222222"` | `{"code": "222222"}` |
| 4 | `token_data.update(additional_data)` | | |
A's token is now stored with B's verification code, while the email A receives contains `"111111"` (the local variable). A's reset then fails, and B's code is bound to A's token.
I want to be straight about severity: this is a race with a narrow window, not a guaranteed leak on every request. But it is on the password-reset path, it needs no attacker to trigger, and the fix is two lines per function.
### Second, unconditional effect
When a caller *does* pass a dict, `additional_data["code"] = code` writes into **the caller's** dict. `api/controllers/console/workspace/members.py:527` passes `additional_data={}` and gets a `"code"` key back that it never asked for.
### Expected behavior
Callers that omit `additional_data` should each get a fresh dict, and a caller's own dict should not be modified.
### Actual behavior
One dict is shared process-wide across all calls that omit the argument, and callers' dicts are mutated in place.
### Note
Three more mutable defaults exist elsewhere and are currently harmless (the functions only read them), but they are the same footgun one edit away from mattering:
- `api/configs/remote_settings_sources/apollo/python_3x.py:24` `http_request(headers={})`
- `api/core/rag/extractor/notion_extractor.py:92` `_get_notion_database_data(query_dict={})`
- `api/services/plugin/oauth_service.py:15` `create_proxy_context(extra_data={})`
I have a PR ready that fixes all six with regression tests.
Contributor guide
Research direction
Start in api/services/account_service.py at the three token generators and their callers, then inspect TokenManager.generate_token in api/libs/helper.py and the referenced worker configuration. Review the existing regression-test changes from the ready PR if available. Done means omitted arguments use independent data, caller-provided dictionaries are not modified, and the token-generator paths remain correct under concurrent requests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python, redis
- Domain
- authentication, backend, security
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 25/100