From 9b137c491d1a0c2ba7c9b2b8fde160c6f4199827 Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Thu, 10 Oct 2019 23:21:05 +0300 Subject: [PATCH 1/2] Always validate outbound entities This should stop invalid payloads being sent out, like shares to the Diaspora network that have no target_guid (due to originating from the AP side). Temporary solution until mock posts are automatically created for better cross-compatibility. Refs: https://git.feneas.org/socialhome/socialhome/issues/522 --- federation/entities/activitypub/mappers.py | 2 + federation/entities/diaspora/entities.py | 4 ++ federation/entities/diaspora/mappers.py | 2 + .../entities/activitypub/test_mappers.py | 29 +++++++++++++- .../tests/entities/diaspora/test_mappers.py | 40 ++++++++++++++++++- .../tests/entities/diaspora/test_utils.py | 3 ++ .../tests/protocols/diaspora/test_protocol.py | 2 + federation/tests/test_outbound.py | 1 + 8 files changed, 80 insertions(+), 3 deletions(-) diff --git a/federation/entities/activitypub/mappers.py b/federation/entities/activitypub/mappers.py index 8b8b7dc..fed87d5 100644 --- a/federation/entities/activitypub/mappers.py +++ b/federation/entities/activitypub/mappers.py @@ -223,6 +223,8 @@ def get_outbound_entity(entity: BaseEntity, private_key): # outbound.parent_signature = outbound.signature if hasattr(outbound, "pre_send"): outbound.pre_send() + # Validate the entity + outbound.validate() return outbound diff --git a/federation/entities/diaspora/entities.py b/federation/entities/diaspora/entities.py index 659f848..28fca4b 100644 --- a/federation/entities/diaspora/entities.py +++ b/federation/entities/diaspora/entities.py @@ -158,6 +158,10 @@ class DiasporaReshare(DiasporaEntityMixin, Share): """Diaspora Reshare.""" _tag_name = "reshare" + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._required += ["target_guid", "target_handle"] + @staticmethod def fill_extra_attributes(attributes): """If `public` is missing, add it as True. diff --git a/federation/entities/diaspora/mappers.py b/federation/entities/diaspora/mappers.py index d334541..7e7efb7 100644 --- a/federation/entities/diaspora/mappers.py +++ b/federation/entities/diaspora/mappers.py @@ -285,4 +285,6 @@ def get_outbound_entity(entity: BaseEntity, private_key: RsaKey): # in all situations but is apparently being removed. # TODO: remove this once Diaspora removes the extra signature outbound.parent_signature = outbound.signature + # Validate the entity + outbound.validate() return outbound diff --git a/federation/tests/entities/activitypub/test_mappers.py b/federation/tests/entities/activitypub/test_mappers.py index 544eb91..6d1b685 100644 --- a/federation/tests/entities/activitypub/test_mappers.py +++ b/federation/tests/entities/activitypub/test_mappers.py @@ -1,4 +1,5 @@ -from unittest.mock import patch +from datetime import datetime +from unittest.mock import patch, Mock import pytest @@ -6,7 +7,7 @@ from federation.entities.activitypub.entities import ( ActivitypubFollow, ActivitypubAccept, ActivitypubProfile, ActivitypubPost, ActivitypubComment, ActivitypubRetraction, ActivitypubShare) from federation.entities.activitypub.mappers import message_to_objects, get_outbound_entity -from federation.entities.base import Accept, Follow, Profile, Post, Comment, Image +from federation.entities.base import Accept, Follow, Profile, Post, Comment, Image, Share from federation.tests.fixtures.payloads import ( ACTIVITYPUB_FOLLOW, ACTIVITYPUB_PROFILE, ACTIVITYPUB_PROFILE_INVALID, ACTIVITYPUB_UNDO_FOLLOW, ACTIVITYPUB_POST, ACTIVITYPUB_COMMENT, ACTIVITYPUB_RETRACTION, ACTIVITYPUB_SHARE, ACTIVITYPUB_RETRACTION_SHARE, @@ -274,20 +275,44 @@ class TestActivitypubEntityMappersReceive: class TestGetOutboundEntity: def test_already_fine_entities_are_returned_as_is(self, private_key): entity = ActivitypubAccept() + entity.validate = Mock() assert get_outbound_entity(entity, private_key) == entity entity = ActivitypubFollow() + entity.validate = Mock() assert get_outbound_entity(entity, private_key) == entity entity = ActivitypubProfile() + entity.validate = Mock() assert get_outbound_entity(entity, private_key) == entity + @patch.object(ActivitypubAccept, "validate", new=Mock()) def test_accept_is_converted_to_activitypubaccept(self, private_key): entity = Accept() assert isinstance(get_outbound_entity(entity, private_key), ActivitypubAccept) + @patch.object(ActivitypubFollow, "validate", new=Mock()) def test_follow_is_converted_to_activitypubfollow(self, private_key): entity = Follow() assert isinstance(get_outbound_entity(entity, private_key), ActivitypubFollow) + @patch.object(ActivitypubProfile, "validate", new=Mock()) def test_profile_is_converted_to_activitypubprofile(self, private_key): entity = Profile() assert isinstance(get_outbound_entity(entity, private_key), ActivitypubProfile) + + def test_entity_is_validated__fail(self, private_key): + entity = Share( + actor_id="https://localhost.local/foo", + id="https://localhost.local/bar", + created_at=datetime.now(), + ) + with pytest.raises(ValueError): + get_outbound_entity(entity, private_key) + + def test_entity_is_validated__success(self, private_key): + entity = Share( + actor_id="https://localhost.local/foo", + id="https://localhost.local/bar", + created_at=datetime.now(), + target_id="https://localhost.local/bar", + ) + get_outbound_entity(entity, private_key) diff --git a/federation/tests/entities/diaspora/test_mappers.py b/federation/tests/entities/diaspora/test_mappers.py index 69c75ae..5acce00 100644 --- a/federation/tests/entities/diaspora/test_mappers.py +++ b/federation/tests/entities/diaspora/test_mappers.py @@ -262,31 +262,40 @@ class TestDiasporaEntityMappersReceive: class TestGetOutboundEntity: def test_already_fine_entities_are_returned_as_is(self, private_key): entity = DiasporaPost() + entity.validate = Mock() assert get_outbound_entity(entity, private_key) == entity entity = DiasporaLike() + entity.validate = Mock() assert get_outbound_entity(entity, private_key) == entity entity = DiasporaComment() + entity.validate = Mock() assert get_outbound_entity(entity, private_key) == entity entity = DiasporaProfile(handle="foobar@example.com", guid="1234") + entity.validate = Mock() assert get_outbound_entity(entity, private_key) == entity entity = DiasporaContact() + entity.validate = Mock() assert get_outbound_entity(entity, private_key) == entity entity = DiasporaReshare() + entity.validate = Mock() assert get_outbound_entity(entity, private_key) == entity + @patch.object(DiasporaPost, "validate", new=Mock()) def test_post_is_converted_to_diasporapost(self, private_key): entity = Post() assert isinstance(get_outbound_entity(entity, private_key), DiasporaPost) + @patch.object(DiasporaComment, "validate", new=Mock()) def test_comment_is_converted_to_diasporacomment(self, private_key): entity = Comment() assert isinstance(get_outbound_entity(entity, private_key), DiasporaComment) + @patch.object(DiasporaLike, "validate", new=Mock()) def test_reaction_of_like_is_converted_to_diasporalike(self, private_key): entity = Reaction(reaction="like") assert isinstance(get_outbound_entity(entity, private_key), DiasporaLike) - + @patch.object(DiasporaProfile, "validate", new=Mock()) def test_profile_is_converted_to_diasporaprofile(self, private_key): entity = Profile(handle="foobar@example.com", guid="1234") assert isinstance(get_outbound_entity(entity, private_key), DiasporaProfile) @@ -301,20 +310,24 @@ class TestGetOutboundEntity: with pytest.raises(ValueError): get_outbound_entity(entity, private_key) + @patch.object(DiasporaRetraction, "validate", new=Mock()) def test_retraction_is_converted_to_diasporaretraction(self, private_key): entity = Retraction() assert isinstance(get_outbound_entity(entity, private_key), DiasporaRetraction) + @patch.object(DiasporaContact, "validate", new=Mock()) def test_follow_is_converted_to_diasporacontact(self, private_key): entity = Follow() assert isinstance(get_outbound_entity(entity, private_key), DiasporaContact) + @patch.object(DiasporaReshare, "validate", new=Mock()) def test_share_is_converted_to_diasporareshare(self, private_key): entity = Share() assert isinstance(get_outbound_entity(entity, private_key), DiasporaReshare) def test_signs_relayable_if_no_signature(self, private_key): entity = DiasporaComment() + entity.validate = Mock() outbound = get_outbound_entity(entity, private_key) assert outbound.signature != "" @@ -323,6 +336,31 @@ class TestGetOutboundEntity: entity.outbound_doc = "foobar" assert get_outbound_entity(entity, private_key) == entity + def test_entity_is_validated__fail(self, private_key): + entity = Share( + actor_id="foobar@localhost.local", + handle="foobar@localhost.local", + id="1"*16, + guid="1"*16, + created_at=datetime.now(), + target_id="2" * 16, + ) + with pytest.raises(ValueError): + get_outbound_entity(entity, private_key) + + def test_entity_is_validated__success(self, private_key): + entity = Share( + actor_id="foobar@localhost.local", + handle="foobar@localhost.local", + id="1" * 16, + guid="1" * 16, + created_at=datetime.now(), + target_handle="barfoo@remote.local", + target_id="2" * 16, + target_guid="2" * 16, + ) + get_outbound_entity(entity, private_key) + def test_check_sender_and_entity_handle_match(): assert not check_sender_and_entity_handle_match("foo", "bar") diff --git a/federation/tests/entities/diaspora/test_utils.py b/federation/tests/entities/diaspora/test_utils.py index 272d321..2da2227 100644 --- a/federation/tests/entities/diaspora/test_utils.py +++ b/federation/tests/entities/diaspora/test_utils.py @@ -1,10 +1,12 @@ import datetime import re +from unittest.mock import patch, Mock import arrow from lxml import etree from federation.entities.base import Post, Profile +from federation.entities.diaspora.entities import DiasporaPost from federation.entities.diaspora.utils import ( get_full_xml_representation, format_dt, add_element_to_doc) from federation.entities.utils import get_base_attributes @@ -30,6 +32,7 @@ class TestGetBaseAttributes: class TestGetFullXMLRepresentation: + @patch.object(DiasporaPost, "validate", new=Mock()) def test_returns_xml_document(self): entity = Post() document = get_full_xml_representation(entity, "") diff --git a/federation/tests/protocols/diaspora/test_protocol.py b/federation/tests/protocols/diaspora/test_protocol.py index 214490e..b5fd0d6 100644 --- a/federation/tests/protocols/diaspora/test_protocol.py +++ b/federation/tests/protocols/diaspora/test_protocol.py @@ -111,6 +111,7 @@ class TestDiasporaProtocol(DiasporaTestBase): mock_me.return_value = mock_me_instance protocol = Protocol() entity = DiasporaPost() + entity.validate = Mock() private_key = get_dummy_private_key() outbound_entity = get_outbound_entity(entity, private_key) data = protocol.build_send(outbound_entity, from_user=UserType( @@ -131,6 +132,7 @@ class TestDiasporaProtocol(DiasporaTestBase): mock_me.return_value = mock_me_instance protocol = Protocol() entity = DiasporaPost() + entity.validate = Mock() private_key = get_dummy_private_key() outbound_entity = get_outbound_entity(entity, private_key) data = protocol.build_send(outbound_entity, to_user_key="public key", from_user=UserType( diff --git a/federation/tests/test_outbound.py b/federation/tests/test_outbound.py index 687f66e..568bb83 100644 --- a/federation/tests/test_outbound.py +++ b/federation/tests/test_outbound.py @@ -16,6 +16,7 @@ class TestHandleCreatePayloadBuildsAPayload: mock_me.return_value = Mock(render=mock_render) author_user = Mock() entity = DiasporaPost() + entity.validate = Mock() handle_create_payload(entity, author_user, "diaspora") mock_render.assert_called_once_with() From ee7f3aef60785a5ac4ee823b99dec456ff66629d Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Fri, 11 Oct 2019 00:13:10 +0300 Subject: [PATCH 2/2] Only validate signatures for inbound entities For now anyway. --- federation/entities/activitypub/mappers.py | 2 +- federation/entities/diaspora/mappers.py | 2 +- federation/entities/mixins.py | 7 ++++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/federation/entities/activitypub/mappers.py b/federation/entities/activitypub/mappers.py index fed87d5..8538f0c 100644 --- a/federation/entities/activitypub/mappers.py +++ b/federation/entities/activitypub/mappers.py @@ -224,7 +224,7 @@ def get_outbound_entity(entity: BaseEntity, private_key): if hasattr(outbound, "pre_send"): outbound.pre_send() # Validate the entity - outbound.validate() + outbound.validate(direction="outbound") return outbound diff --git a/federation/entities/diaspora/mappers.py b/federation/entities/diaspora/mappers.py index 7e7efb7..daff354 100644 --- a/federation/entities/diaspora/mappers.py +++ b/federation/entities/diaspora/mappers.py @@ -286,5 +286,5 @@ def get_outbound_entity(entity: BaseEntity, private_key: RsaKey): # TODO: remove this once Diaspora removes the extra signature outbound.parent_signature = outbound.signature # Validate the entity - outbound.validate() + outbound.validate(direction="outbound") return outbound diff --git a/federation/entities/mixins.py b/federation/entities/mixins.py index d4ba203..01138d4 100644 --- a/federation/entities/mixins.py +++ b/federation/entities/mixins.py @@ -63,14 +63,14 @@ class BaseEntity: """ pass - def validate(self): + def validate(self, direction: str = "inbound") -> None: """Do validation. 1) Check `_required` have been given 2) Make sure all attrs in required have a non-empty value 3) Loop through attributes and call their `validate_` methods, if any. 4) Validate allowed children - 5) Validate signatures + 5) Validate signatures (if inbound) """ attributes = [] validates = [] @@ -86,7 +86,8 @@ class BaseEntity: self._validate_required(attributes) self._validate_attributes(validates) self._validate_children() - self._validate_signatures() + if direction == "inbound": + self._validate_signatures() def _validate_required(self, attributes): """Ensure required attributes are present."""