langgenius / langgenius/dify

Mutable default argument shared across requests in the token generators (password reset path)

Open
#39,851 1 comment 1 reaction 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.