element-hq / element-hq/synapse
Consistently deep copy dict fields for end to end keys
- Dominant language
- Python
- Stars
- 4.6k
- Forks
- 600
- Avg merge
- 5d 22h
- Merged PRs (30d)
- 51
Description
### Description
In `end_to_end_keys.py` in the method `_get_e2e_cross_signing_signatures_txn` dictionaries are copied using `.copy()` to ensure a deepcopy instead of passing the reference as explained in this comment:
```
# We need to copy everything, because the result may have come
# from the cache. dict.copy only does a shallow copy, so we
# need to recursively copy the dicts that will be modified.
```
But this is not done consistently as can be seen in line 1086:
```python
user_sigs = signatures[from_user_id] = signatures[from_user_id]
user_sigs[key_id] = signature
```
I would argue, that we also need to use `.copy()` here otherwise manipulation of this dictionary could lead to an inconsistent/corrupted cache state. This is particularly relevant because the cache stores cryptographic primitives of users.
### Steps to reproduce
I noticed this while reading through the code.
### Homeserver
local homeserver
### Synapse Version
1.157.2
### Installation Method
pip (from PyPI)
### Database
SQLite
### Workers
Single process
### Platform
debian
### Configuration
_No response_
### Relevant log output
```shell
As this is something I noticed in the code, I dont have any logs.
```
### Anything else that would be useful to know?
_No response_
Contributor guide
Research direction
Start in end_to_end_keys.py at _get_e2e_cross_signing_signatures_txn and inspect the assignment around line 1086, alongside the nearby copy-handling comment. Trace whether the dictionary being modified can come from the cache, then verify that the change prevents mutations from affecting cached data and that the relevant end-to-end key behavior remains correct.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100