mirror of
https://github.com/element-hq/synapse.git
synced 2026-08-28 20:28:17 +00:00
Validate room name, avatar and heroes' profile fields before sending down sliding sync as trusted fields
Fixes: https://github.com/element-hq/synapse/security/advisories/GHSA-jhcg-5392-5mjw Fixes: https://github.com/matrix-org/internal-config/issues/1751 I introduce some stricter JSON types that don't break down to `Any` — it seems these have become possible since our last attempt. (I'm pretty sure mypy wouldn't let you do this a few years ago.) Our `dict[str, Any]` type is such a footgun. I'd like to spread this out further, but will do so after the security release. I then use these stricter JSON types on everything the sliding sync handler pulls out of `event.content` and therefore get forced into a bare minimum level of validation, by the type checker. ----- Reviewed-on: https://github.com/element-hq/synapse-private/pull/151
This commit is contained in:
Olivier 'reivilibre
committed by
Olivier 'reivilibre
Olivier 'reivilibre
parent
c3adee3509
commit
0a2456fef3
@@ -57,6 +57,7 @@ from synapse.types import (
|
||||
StrCollection,
|
||||
StreamKeyType,
|
||||
StreamToken,
|
||||
StrictJsonMapping,
|
||||
)
|
||||
from synapse.types.handlers import SLIDING_SYNC_DEFAULT_BUMP_EVENT_TYPES
|
||||
from synapse.types.handlers.sliding_sync import (
|
||||
@@ -953,7 +954,10 @@ class SlidingSyncHandler:
|
||||
)
|
||||
name_event = name_states.get((EventTypes.Name, ""))
|
||||
if name_event is not None:
|
||||
room_name = name_event.content.get("name")
|
||||
name_event_content: StrictJsonMapping = name_event.content
|
||||
unchecked_room_name = name_event_content.get("name")
|
||||
if isinstance(unchecked_room_name, str):
|
||||
room_name = unchecked_room_name
|
||||
|
||||
# We only need the room summary for calculating heroes, however if we do
|
||||
# fetch it then we can use it to calculate `joined_count` and
|
||||
@@ -1356,18 +1360,28 @@ class SlidingSyncHandler:
|
||||
room_avatar: str | None = None
|
||||
avatar_event = room_state.get((EventTypes.RoomAvatar, ""))
|
||||
if avatar_event is not None:
|
||||
room_avatar = avatar_event.content.get("url")
|
||||
room_avatar_content: StrictJsonMapping = avatar_event.content
|
||||
unchecked_room_avatar = room_avatar_content.get("url")
|
||||
if isinstance(unchecked_room_avatar, str):
|
||||
room_avatar = unchecked_room_avatar
|
||||
|
||||
# Assemble heroes: extract the info from the state we just fetched
|
||||
heroes: list[SlidingSyncResult.RoomResult.StrippedHero] = []
|
||||
for hero_user_id in hero_user_ids:
|
||||
member_event = hero_membership_state.get((EventTypes.Member, hero_user_id))
|
||||
if member_event is not None:
|
||||
member_event_content: StrictJsonMapping = member_event.content
|
||||
unchecked_display_name = member_event_content.get("displayname")
|
||||
unchecked_avatar_url = member_event_content.get("avatar_url")
|
||||
heroes.append(
|
||||
SlidingSyncResult.RoomResult.StrippedHero(
|
||||
user_id=hero_user_id,
|
||||
display_name=member_event.content.get("displayname"),
|
||||
avatar_url=member_event.content.get("avatar_url"),
|
||||
display_name=unchecked_display_name
|
||||
if isinstance(unchecked_display_name, str)
|
||||
else None,
|
||||
avatar_url=unchecked_avatar_url
|
||||
if isinstance(unchecked_avatar_url, str)
|
||||
else None,
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
@@ -37,6 +37,7 @@ from typing import (
|
||||
MutableMapping,
|
||||
NoReturn,
|
||||
Optional,
|
||||
Sequence,
|
||||
TypedDict,
|
||||
TypeVar,
|
||||
Union,
|
||||
@@ -102,6 +103,47 @@ JsonMapping = Mapping[str, Any]
|
||||
# A JSON-serialisable object.
|
||||
JsonSerializable = object
|
||||
|
||||
StrictJsonValue = Union[
|
||||
None,
|
||||
bool,
|
||||
int,
|
||||
float,
|
||||
str,
|
||||
"StrictJsonList",
|
||||
"StrictJsonDict",
|
||||
"StrictJsonSequence",
|
||||
"StrictJsonMapping",
|
||||
]
|
||||
"""
|
||||
Type that represents any valid JSON value, recursively.
|
||||
Does not fall back to `Any` at deeper levels, which makes it more safe than `JsonValue`.
|
||||
|
||||
Can also represent immutable mapping and tuple types.
|
||||
(Not sure if we would be better splitting them out.)
|
||||
"""
|
||||
|
||||
StrictJsonList = list["StrictJsonValue"]
|
||||
"""
|
||||
Type that represents a list of any valid JSON value.
|
||||
"""
|
||||
|
||||
StrictJsonDict = dict[str, "StrictJsonValue"]
|
||||
"""
|
||||
Type that represents a dict with string keys (as per JSON) and values of any
|
||||
valid JSON type.
|
||||
Does not fall back to `Any` at deeper levels, which makes it more safe than `JsonDict`.
|
||||
"""
|
||||
|
||||
StrictJsonSequence = Sequence["StrictJsonValue"]
|
||||
"""
|
||||
Like `StrictJsonList` but using a `Sequence` as the collection type.
|
||||
"""
|
||||
|
||||
StrictJsonMapping = Mapping[str, "StrictJsonValue"]
|
||||
"""
|
||||
Like `StrictJsonDict` but using `Mapping` as the collection type.
|
||||
"""
|
||||
|
||||
# Collection[str] that does not include str itself; str being a Sequence[str]
|
||||
# is very misleading and results in bugs.
|
||||
#
|
||||
|
||||
@@ -13,6 +13,7 @@
|
||||
#
|
||||
import logging
|
||||
from typing import Any
|
||||
from unittest.mock import patch
|
||||
|
||||
from parameterized import parameterized, parameterized_class
|
||||
|
||||
@@ -26,7 +27,11 @@ from synapse.server import HomeServer
|
||||
from synapse.util.clock import Clock
|
||||
|
||||
from tests.rest.client.sliding_sync.test_sliding_sync import SlidingSyncBase
|
||||
from tests.test_utils.event_injection import create_event
|
||||
from tests.test_utils.event_injection import (
|
||||
create_event,
|
||||
inject_event,
|
||||
inject_member_event,
|
||||
)
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -1407,3 +1412,239 @@ class SlidingSyncRoomsMetaTestCase(SlidingSyncBase):
|
||||
}
|
||||
}
|
||||
response_body, _ = self.do_sync(sync_body, tok=user1_tok)
|
||||
|
||||
@parameterized.expand(((True,), (None,), ({"a": "dict"},), (["a list"],), (42,)))
|
||||
def test_rooms_meta_non_string_name(self, non_string_name: object) -> None:
|
||||
"""
|
||||
Test that when the room name is not a string, it gets
|
||||
treated the same as if there is no room name set;
|
||||
the `name` field is omitted and `heroes` are populated instead.
|
||||
"""
|
||||
user1_id = self.register_user("user1", "pass")
|
||||
user1_tok = self.login(user1_id, "pass")
|
||||
|
||||
# For heroes to be emitted, we need a second user
|
||||
user2_id = self.register_user("user2", "pass")
|
||||
user2_tok = self.login(user2_id, "pass")
|
||||
|
||||
room_id = self.helper.create_room_as(
|
||||
user1_id,
|
||||
tok=user1_tok,
|
||||
)
|
||||
self.helper.join(room_id, user2_id, tok=user2_tok)
|
||||
|
||||
# Set the room name to a non-string
|
||||
# Need to patch out our client-sent event checks to do this.
|
||||
# (We don't apply these same out-of-spec checks to events
|
||||
# received through federation.
|
||||
# Could have instead set up the test to receive the event over federation.)
|
||||
with patch("synapse.events.validator.EventValidator.validate_new"):
|
||||
self.get_success(
|
||||
inject_event(
|
||||
self.hs,
|
||||
room_id=room_id,
|
||||
sender=user1_id,
|
||||
type=EventTypes.Name,
|
||||
state_key="",
|
||||
content={"name": non_string_name},
|
||||
)
|
||||
)
|
||||
|
||||
sync_body = {
|
||||
"lists": {
|
||||
"wombat": {
|
||||
"ranges": [[0, 1]],
|
||||
"required_state": [],
|
||||
"timeline_limit": 0,
|
||||
}
|
||||
}
|
||||
}
|
||||
response_body, _ = self.do_sync(sync_body, tok=user1_tok)
|
||||
|
||||
# Sanity check that the room is included with an initial snapshot
|
||||
self.assertEqual(response_body["rooms"][room_id]["initial"], True)
|
||||
|
||||
# The name should be omitted (non-string value treated as unset).
|
||||
self.assertNotIn(
|
||||
"name",
|
||||
response_body["rooms"][room_id],
|
||||
response_body["rooms"][room_id],
|
||||
)
|
||||
|
||||
# Since there is no name, heroes should be populated.
|
||||
self.assertEqual(
|
||||
response_body["rooms"][room_id]["heroes"],
|
||||
[{"displayname": "user2", "user_id": "@user2:test"}],
|
||||
)
|
||||
|
||||
@parameterized.expand(((True,), (None,), ({"a": "dict"},), (["a list"],), (42,)))
|
||||
def test_rooms_meta_non_string_avatar(self, non_string_avatar: str) -> None:
|
||||
"""
|
||||
Test that when the room avatar is not a string, it gets
|
||||
treated the same as if there is no room avatar set;
|
||||
the `avatar` field is omitted.
|
||||
"""
|
||||
user1_id = self.register_user("user1", "pass")
|
||||
user1_tok = self.login(user1_id, "pass")
|
||||
|
||||
room_id = self.helper.create_room_as(
|
||||
user1_id,
|
||||
tok=user1_tok,
|
||||
)
|
||||
|
||||
# Set the room avatar to a dict (non-string) instead of a URL string.
|
||||
# Need to patch out our client-sent event checks to do this
|
||||
# (We don't apply these same out-of-spec checks to events
|
||||
# received through federation.
|
||||
# Could have instead set up the test to receive the event over federation.)
|
||||
with patch("synapse.events.validator.EventValidator.validate_new"):
|
||||
self.get_success(
|
||||
inject_event(
|
||||
self.hs,
|
||||
room_id=room_id,
|
||||
sender=user1_id,
|
||||
type=EventTypes.RoomAvatar,
|
||||
state_key="",
|
||||
content={"url": non_string_avatar},
|
||||
)
|
||||
)
|
||||
|
||||
sync_body = {
|
||||
"lists": {
|
||||
"wombat": {
|
||||
"ranges": [[0, 1]],
|
||||
"required_state": [],
|
||||
"timeline_limit": 0,
|
||||
}
|
||||
}
|
||||
}
|
||||
response_body, _ = self.do_sync(sync_body, tok=user1_tok)
|
||||
|
||||
# Sanity check that the room is included with an initial snapshot
|
||||
self.assertEqual(response_body["rooms"][room_id]["initial"], True)
|
||||
|
||||
# The avatar should be omitted (non-string value treated as unset).
|
||||
self.assertNotIn(
|
||||
"avatar",
|
||||
response_body["rooms"][room_id],
|
||||
response_body["rooms"][room_id],
|
||||
)
|
||||
|
||||
@parameterized.expand(((True,), (None,), ({"a": "dict"},), (["a list"],), (42,)))
|
||||
def test_rooms_meta_heroes_non_string_displayname(
|
||||
self, non_string_name: str
|
||||
) -> None:
|
||||
"""
|
||||
Test that when a hero's displayname is not a string, it gets
|
||||
treated the same as if there is no displayname set:
|
||||
the `displayname` field is omitted from the hero entry.
|
||||
"""
|
||||
user1_id = self.register_user("user1", "pass")
|
||||
user1_tok = self.login(user1_id, "pass")
|
||||
user2_id = self.register_user("user2", "pass")
|
||||
|
||||
# Create a room with no name so heroes are populated.
|
||||
room_id = self.helper.create_room_as(
|
||||
user1_id,
|
||||
tok=user1_tok,
|
||||
)
|
||||
|
||||
# Inject a membership event for user2 with a non-string displayname.
|
||||
self.get_success(
|
||||
inject_member_event(
|
||||
self.hs,
|
||||
room_id,
|
||||
sender=user2_id,
|
||||
target=user2_id,
|
||||
membership=Membership.JOIN,
|
||||
extra_content={
|
||||
"displayname": non_string_name,
|
||||
"avatar_url": "mxc://example.org/a-real-mxc",
|
||||
},
|
||||
)
|
||||
)
|
||||
|
||||
sync_body = {
|
||||
"lists": {
|
||||
"wombat": {
|
||||
"ranges": [[0, 1]],
|
||||
"required_state": [],
|
||||
"timeline_limit": 0,
|
||||
}
|
||||
}
|
||||
}
|
||||
response_body, _ = self.do_sync(sync_body, tok=user1_tok)
|
||||
|
||||
# Sanity check that the room is included with an initial snapshot
|
||||
self.assertEqual(response_body["rooms"][room_id]["initial"], True)
|
||||
self.assertNotIn(
|
||||
"name", response_body["rooms"][room_id], response_body["rooms"][room_id]
|
||||
)
|
||||
|
||||
# user2 should be in the heroes list, but without a displayname
|
||||
self.assertEqual(
|
||||
response_body["rooms"][room_id]["heroes"],
|
||||
[
|
||||
{
|
||||
"avatar_url": "mxc://example.org/a-real-mxc",
|
||||
"user_id": "@user2:test",
|
||||
}
|
||||
],
|
||||
)
|
||||
|
||||
@parameterized.expand(((True,), (None,), ({"a": "dict"},), (["a list"],), (42,)))
|
||||
def test_rooms_meta_heroes_non_string_avatar_url(
|
||||
self, non_string_avatar: str
|
||||
) -> None:
|
||||
"""
|
||||
Test that when a hero's avatar URL is not a string, it gets
|
||||
treated the same as if there is no avatar URL set:
|
||||
the `avatar_url` field is omitted from the hero entry.
|
||||
"""
|
||||
user1_id = self.register_user("user1", "pass")
|
||||
user1_tok = self.login(user1_id, "pass")
|
||||
user2_id = self.register_user("user2", "pass")
|
||||
user2_tok = self.login(user2_id, "pass")
|
||||
|
||||
# Create a room with no name so heroes are populated.
|
||||
room_id = self.helper.create_room_as(
|
||||
user2_id,
|
||||
tok=user2_tok,
|
||||
)
|
||||
self.helper.join(room_id, user1_id, tok=user1_tok)
|
||||
|
||||
# Inject a membership event for user2 with a non-string avatar_url.
|
||||
self.get_success(
|
||||
inject_member_event(
|
||||
self.hs,
|
||||
room_id,
|
||||
sender=user2_id,
|
||||
target=user2_id,
|
||||
membership=Membership.JOIN,
|
||||
extra_content={
|
||||
"displayname": "second user",
|
||||
"avatar_url": non_string_avatar,
|
||||
},
|
||||
)
|
||||
)
|
||||
|
||||
sync_body = {
|
||||
"lists": {
|
||||
"wombat": {
|
||||
"ranges": [[0, 1]],
|
||||
"required_state": [],
|
||||
"timeline_limit": 0,
|
||||
}
|
||||
}
|
||||
}
|
||||
response_body, _ = self.do_sync(sync_body, tok=user1_tok)
|
||||
|
||||
# Sanity check that the room is included with an initial snapshot
|
||||
self.assertEqual(response_body["rooms"][room_id]["initial"], True)
|
||||
self.assertNotIn("name", response_body["rooms"][room_id])
|
||||
|
||||
# user2 should be in the heroes list, but without an avatar
|
||||
self.assertEqual(
|
||||
response_body["rooms"][room_id]["heroes"],
|
||||
[{"displayname": "second user", "user_id": "@user2:test"}],
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user