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

Validate device_keys for C-S /keys/query requests #10593

Merged
merged 6 commits into from
Aug 20, 2021
Merged

Validate device_keys for C-S /keys/query requests #10593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
427c0de
Validate device_keys for C-S /keys/query requests
Aug 12, 2021
69116a5
changelog
Aug 12, 2021
f71b822
lint
Aug 13, 2021
c6a24e4
More thorough validation
Aug 17, 2021
588437c
Merge branch 'develop' into dmr/user_query_keys
Aug 20, 2021
f4d710b
Update imports after rest flattening
Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/10593.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Reject Client-Server /keys/query requests which provide device_ids incorrectly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Reject Client-Server /keys/query requests which provide device_ids incorrectly.
Reject Client-Server `/keys/query` requests which provide `device_ids` incorrectly.

8 changes: 8 additions & 0 deletions synapse/api/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,14 @@ def error_dict(self):
return cs_error(self.msg, self.errcode)


class InvalidAPICallError(SynapseError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like if we're going to introduce a new subclass here we should consider using it more widely. (Or at least file an issue about it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. I had in mind #8445 here---e.g. that any systematic approach to validation could make use of this subclass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense as doing it as part of that issue!

"""You called an existing API endpoint, but fed that endpoint
invalid or incomplete data."""

def __init__(self, msg: str):
super().__init__(HTTPStatus.BAD_REQUEST, msg, Codes.BAD_JSON)


class ProxiedRequestError(SynapseError):
"""An error from a general matrix endpoint, eg. from a proxied Matrix API call.

Expand Down
16 changes: 15 additions & 1 deletion synapse/rest/client/keys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@
# limitations under the License.

import logging
from typing import Any

from synapse.api.errors import SynapseError
from synapse.api.errors import InvalidAPICallError, SynapseError
from synapse.http.servlet import (
RestServlet,
parse_integer,
Expand Down Expand Up @@ -163,6 +164,19 @@ async def on_POST(self, request):
device_id = requester.device_id
timeout = parse_integer(request, "timeout", 10 * 1000)
body = parse_json_object_from_request(request)

device_keys = body.get("device_keys")
if not isinstance(device_keys, dict):
raise InvalidAPICallError("'device_keys' must be a JSON object")

def is_list_of_strings(values: Any) -> bool:
return isinstance(values, list) and all(isinstance(v, str) for v in values)

if any(not is_list_of_strings(keys) for keys in device_keys.values()):
raise InvalidAPICallError(
"'device_keys' values must be a list of strings",
)

result = await self.e2e_keys_handler.query_devices(
body, timeout, user_id, device_id
)
Expand Down
77 changes: 77 additions & 0 deletions tests/rest/client/v2_alpha/test_keys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
from http import HTTPStatus
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a license header.


from synapse.api.errors import Codes
from synapse.rest import admin
from synapse.rest.client import keys, login

from tests import unittest


class KeyQueryTestCase(unittest.HomeserverTestCase):
servlets = [
keys.register_servlets,
admin.register_servlets_for_client_rest_resource,
login.register_servlets,
]

def test_rejects_device_id_ice_key_outside_of_list(self):
self.register_user("alice", "wonderland")
alice_token = self.login("alice", "wonderland")
bob = self.register_user("bob", "uncle")
channel = self.make_request(
"POST",
"/_matrix/client/r0/keys/query",
{
"device_keys": {
bob: "device_id1",
},
},
alice_token,
)
self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
self.assertEqual(
channel.json_body["errcode"],
Codes.BAD_JSON,
channel.result,
)

def test_rejects_device_key_given_as_map_to_bool(self):
self.register_user("alice", "wonderland")
alice_token = self.login("alice", "wonderland")
bob = self.register_user("bob", "uncle")
channel = self.make_request(
"POST",
"/_matrix/client/r0/keys/query",
{
"device_keys": {
bob: {
"device_id1": True,
},
},
},
alice_token,
)

self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
self.assertEqual(
channel.json_body["errcode"],
Codes.BAD_JSON,
channel.result,
)

def test_requires_device_key(self):
"""`device_keys` is required. We should complain if it's missing."""
self.register_user("alice", "wonderland")
alice_token = self.login("alice", "wonderland")
channel = self.make_request(
"POST",
"/_matrix/client/r0/keys/query",
{},
alice_token,
)
self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
self.assertEqual(
channel.json_body["errcode"],
Codes.BAD_JSON,
channel.result,
)