diff --git a/.github/changelog/fix-follow-target b/.github/changelog/fix-follow-target new file mode 100644 index 0000000000..04fcbf04ba --- /dev/null +++ b/.github/changelog/fix-follow-target @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Follow requests are now only accepted when they are meant for the account that received them, and profile links on other sites no longer count as accounts on your site. diff --git a/includes/class-application.php b/includes/class-application.php index 244a58a171..e4671f2773 100644 --- a/includes/class-application.php +++ b/includes/class-application.php @@ -298,18 +298,8 @@ public static function is_application_resource( $uri ) { list( $identifier, $host ) = $identifier_and_host; - /* - * The resource must point at this site, or at its pre-migration host. Both sides are folded: - * the requested host is whatever the caller typed, and the stored ones are whatever an admin did. - */ - $host = normalize_host( fold_host( $host ) ); - - // A host of `.` or `[]` folds away to nothing, which would otherwise match an unset old host. - if ( '' === $host ) { - return false; - } - - if ( normalize_host( fold_host( home_host() ) ) !== $host && normalize_host( fold_host( \get_option( 'activitypub_old_host' ) ) ) !== $host ) { + // The resource must point at this site, or at its pre-migration host. + if ( ! is_local_host( $host ) ) { return false; } diff --git a/includes/collection/class-actors.php b/includes/collection/class-actors.php index 5db88c580e..90c868f67a 100644 --- a/includes/collection/class-actors.php +++ b/includes/collection/class-actors.php @@ -13,9 +13,8 @@ use Activitypub\Model\User; use Activitypub\Signature; -use function Activitypub\fold_host; +use function Activitypub\is_local_host; use function Activitypub\is_user_type_disabled; -use function Activitypub\normalize_host; use function Activitypub\normalize_url; use function Activitypub\object_to_uri; use function Activitypub\url_to_authorid; @@ -239,13 +238,18 @@ public static function get_id_by_resource( $uri ) { // Extract the scheme, defaulting to `acct` for bare `user@host` handles. $scheme = \wp_parse_url( $uri, PHP_URL_SCHEME ) ?: 'acct'; - // @todo: handle old domain URIs here before we serve a new domain below when we shouldn't. - // Although maybe passing through to ::get_by_username() is enough? - switch ( $scheme ) { // Check for http(s) URIs. case 'http': case 'https': + if ( ! is_local_host( \wp_parse_url( $uri, \PHP_URL_HOST ) ) ) { + return new \WP_Error( + 'activitypub_no_user_found', + \__( 'Actor not found', 'activitypub' ), + array( 'status' => 404 ) + ); + } + // Check for http(s)://blog.example.com/@username. $resource_path = \wp_parse_url( $uri, PHP_URL_PATH ); @@ -289,11 +293,8 @@ public static function get_id_by_resource( $uri ) { case 'acct': $uri = \str_replace( 'acct:', '', $uri ); $identifier = \substr( $uri, 0, \strrpos( $uri, '@' ) ); - $host = normalize_host( fold_host( \substr( \strrchr( $uri, '@' ), 1 ) ) ); - $blog_host = normalize_host( fold_host( \wp_parse_url( \home_url( '/' ), \PHP_URL_HOST ) ) ); - // A host of `.` or `[]` folds away to nothing, which would otherwise match an unset old host. - if ( '' === $host || ( $blog_host !== $host && normalize_host( fold_host( \get_option( 'activitypub_old_host' ) ) ) !== $host ) ) { + if ( ! is_local_host( \substr( (string) \strrchr( $uri, '@' ), 1 ) ) ) { return new \WP_Error( 'activitypub_wrong_host', \__( 'Resource host does not match blog host', 'activitypub' ), diff --git a/includes/functions-url.php b/includes/functions-url.php index 693bb809eb..e93de9a280 100644 --- a/includes/functions-url.php +++ b/includes/functions-url.php @@ -235,6 +235,32 @@ function get_authority( $url ) { return $parts['host']; } +/** + * Check if a host is this site's host or the host it moved from. + * + * Both sides are folded before they are compared: the host is whatever a caller sent, and the stored + * ones are whatever an admin entered. A host of `.` or `[]` folds away to nothing and is dropped, so + * it never matches an unset old host. + * + * @since unreleased + * + * @param string|null $host The host. + * + * @return bool True if the host is local, false otherwise. + */ +function is_local_host( $host ) { + $local = \array_filter( + \array_map( + function ( $local_host ) { + return normalize_host( fold_host( $local_host ) ); + }, + array( home_host(), \get_option( 'activitypub_old_host' ) ) + ) + ); + + return \in_array( normalize_host( fold_host( $host ) ), $local, true ); +} + /** * Check if a URL is from the same domain as the site. * diff --git a/includes/handler/class-follow.php b/includes/handler/class-follow.php index 228377fd50..7cec9d6441 100644 --- a/includes/handler/class-follow.php +++ b/includes/handler/class-follow.php @@ -43,8 +43,12 @@ public static function init() { * @return void */ public static function handle_follow( $activity, $user_ids ) { - // Extract the user ID (follow requests are always for a single user). - $user_id = \is_array( $user_ids ) ? \reset( $user_ids ) : $user_ids; + // Only a Follow of an actor that received it, and the follower is added to that actor. + $user_id = Actors::get_id_by_resource( $activity['object'] ?? '' ); + if ( \is_wp_error( $user_id ) || ! \in_array( (int) $user_id, \array_map( 'intval', (array) $user_ids ), true ) ) { + return; + } + $user_id = (int) $user_id; // Check if the actor already follows the user. $already_following = false; diff --git a/tests/phpunit/tests/includes/class-test-functions-url.php b/tests/phpunit/tests/includes/class-test-functions-url.php index e8c609c968..6d2ef94844 100644 --- a/tests/phpunit/tests/includes/class-test-functions-url.php +++ b/tests/phpunit/tests/includes/class-test-functions-url.php @@ -250,4 +250,44 @@ public function authority_provider() { 'no host' => array( '/inbox', '' ), ); } + + /** + * A host is local when it folds to this site's host or to the host the site moved from. + * + * @covers \Activitypub\is_local_host + * @dataProvider local_host_provider + * + * @param string|null $host The host, with `%s` standing for this site's host. + * @param string|null $old_host The stored old host, or null for none. + * @param bool $expected The expected result. + */ + public function test_is_local_host( $host, $old_host, $expected ) { + if ( null !== $old_host ) { + \update_option( 'activitypub_old_host', $old_host ); + } + + $result = \Activitypub\is_local_host( null === $host ? null : \sprintf( $host, \Activitypub\home_host() ) ); + + \delete_option( 'activitypub_old_host' ); + + $this->assertSame( $expected, $result ); + } + + /** + * Hosts for test_is_local_host. + * + * @return array[] + */ + public function local_host_provider() { + return array( + 'home host' => array( '%s', null, true ), + 'home host, other case' => array( 'WWW.%s.', null, true ), + 'old host' => array( 'old.example', 'old.example', true ), + 'old host, unset' => array( 'old.example', null, false ), + 'foreign host' => array( 'evil.example', 'old.example', false ), + 'empty host' => array( '', null, false ), + 'host that folds away' => array( '.', null, false ), + 'no host' => array( null, null, false ), + ); + } } diff --git a/tests/phpunit/tests/includes/collection/class-test-actors.php b/tests/phpunit/tests/includes/collection/class-test-actors.php index 24bbd110c9..0f552548d6 100644 --- a/tests/phpunit/tests/includes/collection/class-test-actors.php +++ b/tests/phpunit/tests/includes/collection/class-test-actors.php @@ -93,11 +93,12 @@ public function test_get_by_various( $item, $expected ) { $path = wp_parse_url( $item, PHP_URL_PATH ) ?? ''; if ( str_starts_with( $path, '/blog/' ) ) { + $host = wp_parse_url( $item, PHP_URL_HOST ); add_filter( 'home_url', // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable, Generic.CodeAnalysis.UnusedFunctionParameter.Found - function ( $url ) { - return 'http://example.org/blog/'; + function ( $url ) use ( $host ) { + return 'http://' . $host . '/blog/'; } ); } @@ -228,6 +229,8 @@ public function the_resource_provider() { array( \rtrim( $https_home_url, '/' ), 'Activitypub\Model\Blog' ), array( $home_url . '/@blog/s', 'WP_Error' ), array( $home_url . '/@blogs/', 'WP_Error' ), + array( 'https://remote.example/@admin', 'WP_Error' ), + array( 'https://remote.example/@blog', 'WP_Error' ), ); } @@ -268,7 +271,7 @@ public function test_get_type_by_id() { public function test_disabled_blog_profile() { \update_option( 'activitypub_actor_mode', ACTIVITYPUB_ACTOR_AND_BLOG_MODE ); - $resource = 'http://example.org/@blog'; + $resource = \home_url( '/@blog' ); $this->assertEquals( 'Activitypub\Model\Blog', get_class( Actors::get_by_resource( $resource ) ) ); diff --git a/tests/phpunit/tests/includes/handler/class-test-follow.php b/tests/phpunit/tests/includes/handler/class-test-follow.php index eeefc42cac..d762307aaa 100644 --- a/tests/phpunit/tests/includes/handler/class-test-follow.php +++ b/tests/phpunit/tests/includes/handler/class-test-follow.php @@ -547,4 +547,145 @@ public function test_new_hook_fires() { \remove_action( 'activitypub_handled_follow', $new_hook_callback ); \remove_filter( 'pre_get_remote_metadata_by_actor', $mock_metadata_callback ); } + + /** + * A Follow of another actor is not added to the user whose inbox received it. + * + * @covers ::handle_follow + */ + public function test_handle_follow_ignores_other_object() { + $actor_url = 'https://example.com/other-object-actor'; + + $mock_metadata_callback = function () use ( $actor_url ) { + return array( + 'id' => $actor_url, + 'type' => 'Person', + 'preferredUsername' => 'otherobject', + 'inbox' => 'https://example.com/inbox', + ); + }; + \add_filter( 'pre_get_remote_metadata_by_actor', $mock_metadata_callback ); + + $handled_before = \did_action( 'activitypub_handled_follow' ); + + Follow::handle_follow( + array( + 'id' => $actor_url . '/activity/1', + 'type' => 'Follow', + 'actor' => $actor_url, + 'object' => 'https://remote.example/users/bob', + ), + self::$user_id + ); + + \remove_filter( 'pre_get_remote_metadata_by_actor', $mock_metadata_callback ); + + $this->assertCount( 0, Followers::get_many( self::$user_id ), 'A Follow of another actor should not add a follower.' ); + $this->assertSame( $handled_before, \did_action( 'activitypub_handled_follow' ), 'A Follow of another actor should not be handled.' ); + } + + /** + * With several recipients, the follower is added to the one the Follow names, not the first. + * + * @covers ::handle_follow + */ + public function test_handle_follow_picks_the_named_recipient() { + $other_id = self::factory()->user->create( array( 'role' => 'author' ) ); + $actor_url = 'https://example.com/users/several-recipients'; + + $mock_metadata_callback = function () use ( $actor_url ) { + return array( + 'id' => $actor_url, + 'type' => 'Person', + 'preferredUsername' => 'several', + 'inbox' => 'https://example.com/inbox', + ); + }; + \add_filter( 'pre_get_remote_metadata_by_actor', $mock_metadata_callback ); + + Follow::handle_follow( + array( + 'id' => $actor_url . '/activity/1', + 'type' => 'Follow', + 'actor' => $actor_url, + 'object' => Actors::get_by_id( self::$user_id )->get_id(), + ), + array( $other_id, self::$user_id ) + ); + + \remove_filter( 'pre_get_remote_metadata_by_actor', $mock_metadata_callback ); + + $this->assertCount( 1, Followers::get_many( self::$user_id ), 'The named recipient should get the follower.' ); + $this->assertCount( 0, Followers::get_many( $other_id ), 'The other recipient should not.' ); + } + + /** + * A Follow names its object in any form the local actor can be addressed by. + * + * @dataProvider object_form_provider + * @covers ::handle_follow + * + * @param string $form The way the Follow names the local actor. + * @param bool $is_blog Whether the Follow is for the blog actor. + */ + public function test_handle_follow_accepts_object_forms( $form, $is_blog ) { + \update_option( 'activitypub_actor_mode', ACTIVITYPUB_ACTOR_AND_BLOG_MODE ); + + $user_id = $is_blog ? Actors::BLOG_USER_ID : self::$user_id; + $actor = Actors::get_by_id( $user_id ); + $objects = array( + 'id' => $actor->get_id(), + 'inline' => array( + 'id' => $actor->get_id(), + 'type' => 'Person', + ), + 'acct' => 'acct:' . $actor->get_webfinger(), + 'at_name' => \home_url( '/@' . $actor->get_preferred_username() ), + 'home_url' => \home_url( '/' ), + ); + + $actor_url = 'https://example.com/users/' . $form . ( $is_blog ? '-blog' : '' ); + $mock_metadata_callback = function () use ( $actor_url ) { + return array( + 'id' => $actor_url, + 'type' => 'Person', + 'preferredUsername' => 'follower', + 'inbox' => 'https://example.com/inbox', + ); + }; + \add_filter( 'pre_get_remote_metadata_by_actor', $mock_metadata_callback ); + + Follow::handle_follow( + array( + 'id' => $actor_url . '/activity/1', + 'type' => 'Follow', + 'actor' => $actor_url, + 'object' => $objects[ $form ], + ), + $user_id + ); + + \remove_filter( 'pre_get_remote_metadata_by_actor', $mock_metadata_callback ); + \delete_option( 'activitypub_actor_mode' ); + + $this->assertCount( 1, Followers::get_many( $user_id ), 'A Follow of the local actor should add a follower.' ); + } + + /** + * Data provider for test_handle_follow_accepts_object_forms. + * + * @return array The object forms. + */ + public function object_form_provider() { + return array( + 'actor id' => array( 'id', false ), + 'inline object' => array( 'inline', false ), + 'acct uri' => array( 'acct', false ), + 'at-name url' => array( 'at_name', false ), + 'blog actor id' => array( 'id', true ), + 'blog acct uri' => array( 'acct', true ), + 'blog at-name' => array( 'at_name', true ), + 'blog home url' => array( 'home_url', true ), + ); + } } diff --git a/tests/phpunit/tests/includes/rest/class-test-actors-inbox-controller.php b/tests/phpunit/tests/includes/rest/class-test-actors-inbox-controller.php index b921865b2d..eff15927a6 100644 --- a/tests/phpunit/tests/includes/rest/class-test-actors-inbox-controller.php +++ b/tests/phpunit/tests/includes/rest/class-test-actors-inbox-controller.php @@ -9,6 +9,7 @@ use Activitypub\Application; use Activitypub\Collection\Actors; +use Activitypub\Collection\Followers; use Activitypub\Collection\Inbox as Inbox_Collection; use Activitypub\Collection\Outbox; use Activitypub\Handler\Feature_Request; @@ -233,6 +234,45 @@ public function test_follow_request() { \remove_filter( 'activitypub_defer_signature_verification', '__return_true' ); } + /** + * A Follow of another server's actor, delivered to a local inbox, adds no follower. + * + * @covers ::create_item + */ + public function test_follow_of_other_actor_adds_no_follower() { + \add_filter( 'activitypub_defer_signature_verification', '__return_true' ); + + $actor_url = 'https://example.com/users/alice'; + $mock_metadata_callback = function () use ( $actor_url ) { + return array( + 'id' => $actor_url, + 'type' => 'Person', + 'preferredUsername' => 'alice', + 'inbox' => 'https://example.com/inbox', + ); + }; + \add_filter( 'pre_get_remote_metadata_by_actor', $mock_metadata_callback ); + + $json = array( + 'id' => 'https://example.com/activities/follow-bob', + 'type' => 'Follow', + 'actor' => $actor_url, + 'object' => 'https://remote.example/actors/1', + ); + + $request = new \WP_REST_Request( 'POST', '/' . ACTIVITYPUB_REST_NAMESPACE . '/actors/' . self::$user_id . '/inbox' ); + $request->set_header( 'Content-Type', 'application/activity+json' ); + $request->set_body( \wp_json_encode( $json ) ); + + $response = \rest_do_request( $request ); + + \remove_filter( 'pre_get_remote_metadata_by_actor', $mock_metadata_callback ); + \remove_filter( 'activitypub_defer_signature_verification', '__return_true' ); + + $this->assertEquals( 202, $response->get_status() ); + $this->assertCount( 0, Followers::get_many( self::$user_id ), 'A Follow of another actor should not add a follower.' ); + } + /** * Test that deliveries to the retired Application actor's inbox are handled by the shared inbox. *