Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Synapse accepts objects in place of device ids for /_matrix/client/r0/keys/query #10354

Closed
richvdh opened this issue Jul 9, 2021 · 3 comments · Fixed by #10593
Closed

Synapse accepts objects in place of device ids for /_matrix/client/r0/keys/query #10354

richvdh opened this issue Jul 9, 2021 · 3 comments · Fixed by #10593
Assignees
Labels
good first issue Good for newcomers T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@richvdh
Copy link
Member

richvdh commented Jul 9, 2021

https://matrix.org/docs/spec/client_server/r0.6.1#post-matrix-client-r0-keys-query says that device_keys must be a map from string to a list of strings (the device IDs).

However, Synapse erroneously accepts other types in place of that list of strings (in particular, it accepts other maps).

In short: this is accepted as a valid request, where it should be rejected with a 400:

POST /_matrix/client/r0/keys/query HTTP/1.1

{
    "device_keys": {
        "@bob:hs1": {
            "device_id1": true,
        }
    }
}
@richvdh
Copy link
Member Author

richvdh commented Jul 9, 2021

Note that Element iOS used to use the erroneous format: element-hq/element-ios#3539.

However, that was fixed in December 2020 so I think we should be fine to fix this in Synapse.

@richvdh
Copy link
Member Author

richvdh commented Jul 9, 2021

(note that complement has a test for this, which is currently blacklisted for synapse)

@anoadragon453 anoadragon453 added T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. good first issue Good for newcomers labels Jul 9, 2021
@DMRobertson DMRobertson self-assigned this Aug 11, 2021
@DMRobertson
Copy link
Contributor

device_keys is also meant to be required, according to the spec, but I don't think that's enforced by Synapse.

More generally I think this falls under the umbrella of #8445.

DMRobertson pushed a commit that referenced this issue Aug 12, 2021
Closes #10354

A small, not particularly critical fix. I'm interested in seeing if we
can find a more systematic approach though.

Wishlist:
1. Declaratively specify the data we expect
2. Automatic validation
3. Gradually roll this out across synapse
4. don't eat too many CPU cycles
5. type hints available to Python to mypy/PyCharm etc can make use of the data

A quick search mentions jsonschema, fastjsonschema, pydantic. Attrs has
this in but I think it's quite verbose to get validation?
DMRobertson pushed a commit that referenced this issue Aug 20, 2021
* Validate device_keys for C-S /keys/query requests

Closes #10354

A small, not particularly critical fix. I'm interested in seeing if we
can find a more systematic approach though. #8445 is the place for any discussion.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants