diff --git a/CHANGELOG.md b/CHANGELOG.md index b9d64227d..8bb105bd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +# [v1.6.3](https://github.com/flarum/framework/compare/v1.6.2...v1.6.3) +### Fixed +* Post mentions can be used to read any post on the forum without access control (ab1c868b978e8b0d09a5d682c54665dae17d0985). +* Notifications can leak restricted content (d0a2b95dca57d3dae9a0d77b610b1cb1d0b1766a). +* Any user including unactivated can reply in public discussions whose first post was permanently deleted (12f14112a0ecd1484d97330b82beb2a145919015). +* (subscriptions) Post notifications not getting access checked (https://github.com/flarum/framework/commit/e5f05166a062a9a6eb7c12e28728bfd5db7270e3). + ## [v1.6.2](https://github.com/flarum/framework/compare/v1.6.1...v1.6.2) ### Fixed * XSS Vulnerability in core (https://github.com/flarum/framework/pull/3684). diff --git a/extensions/mentions/composer.json b/extensions/mentions/composer.json index 04435e0ce..ff510ef85 100644 --- a/extensions/mentions/composer.json +++ b/extensions/mentions/composer.json @@ -19,7 +19,7 @@ } ], "require": { - "flarum/core": "^1.6" + "flarum/core": "^1.6.3" }, "autoload": { "psr-4": { diff --git a/extensions/mentions/extend.php b/extensions/mentions/extend.php index 794503693..21efec757 100644 --- a/extensions/mentions/extend.php +++ b/extensions/mentions/extend.php @@ -91,11 +91,9 @@ return [ ]), (new Extend\ApiController(Controller\CreatePostController::class)) - ->addInclude(['mentionsPosts', 'mentionsPosts.mentionedBy']) ->addOptionalInclude('mentionsGroups'), (new Extend\ApiController(Controller\UpdatePostController::class)) - ->addInclude(['mentionsPosts', 'mentionsPosts.mentionedBy']) ->addOptionalInclude('mentionsGroups'), (new Extend\ApiController(Controller\AbstractSerializeController::class)) diff --git a/extensions/mentions/src/ConfigureMentions.php b/extensions/mentions/src/ConfigureMentions.php index ef7780755..8378e3285 100644 --- a/extensions/mentions/src/ConfigureMentions.php +++ b/extensions/mentions/src/ConfigureMentions.php @@ -11,7 +11,7 @@ namespace Flarum\Mentions; use Flarum\Group\Group; use Flarum\Http\UrlGenerator; -use Flarum\Post\CommentPost; +use Flarum\Post\PostRepository; use Flarum\Settings\SettingsRepositoryInterface; use Flarum\User\User; use Illuminate\Support\Str; @@ -115,7 +115,8 @@ class ConfigureMentions $tag->filterChain ->prepend([static::class, 'addPostId']) - ->setJS('function(tag) { return flarum.extensions["flarum-mentions"].filterPostMentions(tag); }'); + ->setJS('function(tag) { return flarum.extensions["flarum-mentions"].filterPostMentions(tag); }') + ->addParameterByName('actor'); $config->Preg->match('/\B@["|“](?((?!"#[a-z]{0,3}[0-9]+).)+)["|”]#p(?[0-9]+)\b/', $tagName); } @@ -124,9 +125,11 @@ class ConfigureMentions * @param $tag * @return bool */ - public static function addPostId($tag) + public static function addPostId($tag, User $actor) { - $post = CommentPost::find($tag->getAttribute('id')); + $post = resolve(PostRepository::class) + ->queryVisibleTo($actor) + ->find($tag->getAttribute('id')); if ($post) { $tag->setAttribute('discussionid', (int) $post->discussion_id); diff --git a/extensions/mentions/tests/integration/api/PostMentionsTest.php b/extensions/mentions/tests/integration/api/PostMentionsTest.php index b0396a3d7..9bb48d8c0 100644 --- a/extensions/mentions/tests/integration/api/PostMentionsTest.php +++ b/extensions/mentions/tests/integration/api/PostMentionsTest.php @@ -38,6 +38,7 @@ class PostMentionsTest extends TestCase ], 'discussions' => [ ['id' => 2, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 3, 'first_post_id' => 4, 'comment_count' => 2], + ['id' => 50, 'title' => __CLASS__, 'is_private' => true, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 3, 'first_post_id' => 4, 'comment_count' => 1], ], 'posts' => [ ['id' => 4, 'number' => 2, 'discussion_id' => 2, 'created_at' => Carbon::now(), 'user_id' => 3, 'type' => 'comment', 'content' => '@tobyuuu#5'], @@ -49,6 +50,9 @@ class PostMentionsTest extends TestCase ['id' => 10, 'number' => 11, 'discussion_id' => 2, 'created_at' => Carbon::now(), 'user_id' => 4, 'type' => 'comment', 'content' => '@"Bad "#p6 User"#p9'], ['id' => 11, 'number' => 12, 'discussion_id' => 2, 'created_at' => Carbon::now(), 'user_id' => 40, 'type' => 'comment', 'content' => '@"Bad "#p6 User"#p9'], ['id' => 12, 'number' => 13, 'discussion_id' => 2, 'created_at' => Carbon::now(), 'user_id' => 4, 'type' => 'comment', 'content' => '@"acme"#p11'], + + // Restricted access + ['id' => 50, 'number' => 1, 'discussion_id' => 50, 'created_at' => Carbon::now(), 'user_id' => 3, 'type' => 'comment', 'content' => 'no'], ], 'post_mentions_post' => [ ['post_id' => 4, 'mentions_post_id' => 5], @@ -128,6 +132,37 @@ class PostMentionsTest extends TestCase $this->assertNotNull(CommentPost::find($response['data']['id'])->mentionsPosts->find(4)); } + /** + * @test + */ + public function cannot_mention_a_post_without_access() + { + $response = $this->send( + $this->request('POST', '/api/posts', [ + 'authenticatedAs' => 1, + 'json' => [ + 'data' => [ + 'attributes' => [ + 'content' => '@"potato"#p50', + ], + 'relationships' => [ + 'discussion' => ['data' => ['id' => 2]], + ], + ], + ], + ]) + ); + + $this->assertEquals(201, $response->getStatusCode()); + + $response = json_decode($response->getBody(), true); + + $this->assertStringContainsString('potato', $response['data']['attributes']['contentHtml']); + $this->assertEquals('@"potato"#p50', $response['data']['attributes']['content']); + $this->assertStringNotContainsString('PostMention', $response['data']['attributes']['contentHtml']); + $this->assertNull(CommentPost::find($response['data']['id'])->mentionsPosts->find(50)); + } + /** * @test */ diff --git a/extensions/subscriptions/composer.json b/extensions/subscriptions/composer.json index 2f619beea..d6075c3d4 100644 --- a/extensions/subscriptions/composer.json +++ b/extensions/subscriptions/composer.json @@ -19,7 +19,7 @@ } ], "require": { - "flarum/core": "^1.6" + "flarum/core": "^1.6.3" }, "autoload": { "psr-4": { diff --git a/extensions/subscriptions/extend.php b/extensions/subscriptions/extend.php index c910e7296..e39da0de5 100644 --- a/extensions/subscriptions/extend.php +++ b/extensions/subscriptions/extend.php @@ -21,6 +21,7 @@ use Flarum\Post\Event\Posted; use Flarum\Post\Event\Restored; use Flarum\Subscriptions\HideIgnoredFromAllDiscussionsPage; use Flarum\Subscriptions\Listener; +use Flarum\Subscriptions\Notification\FilterVisiblePostsBeforeSending; use Flarum\Subscriptions\Notification\NewPostBlueprint; use Flarum\Subscriptions\Query\SubscriptionFilterGambit; @@ -36,7 +37,8 @@ return [ ->namespace('flarum-subscriptions', __DIR__.'/views'), (new Extend\Notification()) - ->type(NewPostBlueprint::class, BasicDiscussionSerializer::class, ['alert', 'email']), + ->type(NewPostBlueprint::class, BasicDiscussionSerializer::class, ['alert', 'email']) + ->beforeSending(FilterVisiblePostsBeforeSending::class), (new Extend\ApiSerializer(DiscussionSerializer::class)) ->attribute('subscription', function (DiscussionSerializer $serializer, Discussion $discussion) { diff --git a/extensions/subscriptions/src/Notification/FilterVisiblePostsBeforeSending.php b/extensions/subscriptions/src/Notification/FilterVisiblePostsBeforeSending.php new file mode 100644 index 000000000..b913d9193 --- /dev/null +++ b/extensions/subscriptions/src/Notification/FilterVisiblePostsBeforeSending.php @@ -0,0 +1,35 @@ +post->isVisibleTo($recipient)) { + $newRecipients[] = $recipient; + } + } + + return $newRecipients; + } + + return $recipients; + } +} diff --git a/extensions/subscriptions/tests/integration/api/discussions/ReplyNotificationTest.php b/extensions/subscriptions/tests/integration/api/discussions/ReplyNotificationTest.php index 0600865b7..ea1aba479 100644 --- a/extensions/subscriptions/tests/integration/api/discussions/ReplyNotificationTest.php +++ b/extensions/subscriptions/tests/integration/api/discussions/ReplyNotificationTest.php @@ -10,7 +10,9 @@ namespace Flarum\Subscriptions\tests\integration\api\discussions; use Carbon\Carbon; +use Flarum\Extend\ModelVisibility; use Flarum\Group\Group; +use Flarum\Post\Post; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; use Flarum\User\User; @@ -278,4 +280,47 @@ class ReplyNotificationTest extends TestCase $this->assertEquals(1, $mainUser->getUnreadNotificationCount()); } + + /** @test */ + public function replying_to_a_discussion_with_a_restricted_post_only_sends_notifications_to_allowed_users() + { + // Add visibility scoper to only allow admin + // to see expected new post with content containing 'restricted-test-post'. + $this->extend( + (new ModelVisibility(Post::class)) + ->scope(function (User $actor, $query) { + if (! $actor->isAdmin()) { + $query->where('content', 'not like', '%restricted-test-post%'); + } + }) + ); + + $this->app(); + + /** @var User $allowedUser */ + $allowedUser = User::query()->find(1); + $normalUser = User::query()->find(2); + + $this->assertEquals(0, $allowedUser->getUnreadNotificationCount()); + $this->assertEquals(0, $normalUser->getUnreadNotificationCount()); + + $this->send( + $this->request('POST', '/api/posts', [ + 'authenticatedAs' => 3, + 'json' => [ + 'data' => [ + 'attributes' => [ + 'content' => 'restricted-test-post', + ], + 'relationships' => [ + 'discussion' => ['data' => ['id' => 1]], + ], + ], + ], + ]) + ); + + $this->assertEquals(1, $allowedUser->getUnreadNotificationCount()); + $this->assertEquals(0, $normalUser->getUnreadNotificationCount()); + } } diff --git a/framework/core/src/Discussion/Command/StartDiscussionHandler.php b/framework/core/src/Discussion/Command/StartDiscussionHandler.php index bda8cb439..25f7b09bd 100644 --- a/framework/core/src/Discussion/Command/StartDiscussionHandler.php +++ b/framework/core/src/Discussion/Command/StartDiscussionHandler.php @@ -79,7 +79,7 @@ class StartDiscussionHandler // We will do this by running the PostReply command. try { $post = $this->bus->dispatch( - new PostReply($discussion->id, $actor, $data, $ipAddress) + new PostReply($discussion->id, $actor, $data, $ipAddress, true) ); } catch (Exception $e) { $discussion->delete(); diff --git a/framework/core/src/Formatter/Formatter.php b/framework/core/src/Formatter/Formatter.php index 2db336cd2..2176cfb69 100644 --- a/framework/core/src/Formatter/Formatter.php +++ b/framework/core/src/Formatter/Formatter.php @@ -91,6 +91,13 @@ class Formatter { $parser = $this->getParser($context); + /* + * Can be injected in tag or attribute filters by calling: + * ->addParameterByName('actor') on the filter. + * See the mentions extension's ConfigureMentions.php for an example. + */ + $parser->registeredVars['actor'] = $user; + foreach ($this->parsingCallbacks as $callback) { $text = $callback($parser, $context, $text, $user); } diff --git a/framework/core/src/Notification/NotificationSyncer.php b/framework/core/src/Notification/NotificationSyncer.php index 18d845a8d..752538cab 100644 --- a/framework/core/src/Notification/NotificationSyncer.php +++ b/framework/core/src/Notification/NotificationSyncer.php @@ -9,6 +9,7 @@ namespace Flarum\Notification; +use Flarum\Database\AbstractModel; use Flarum\Notification\Blueprint\BlueprintInterface; use Flarum\Notification\Driver\NotificationDriverInterface; use Flarum\User\User; @@ -74,6 +75,12 @@ class NotificationSyncer continue; } + // To add access checking on notification subjects, we first attempt + // to load visible subjects to this user. + if (! $this->userCanSeeSubject($user, $blueprint->getSubject())) { + continue; + } + $existing = $toDelete->first(function ($notification) use ($user) { return $notification->user_id === $user->id; }); @@ -161,6 +168,18 @@ class NotificationSyncer Notification::whereIn('id', $ids)->update(['is_deleted' => $isDeleted]); } + /** + * Check access to determine if the recipient is allowed to receive the notification. + */ + protected function userCanSeeSubject(User $user, ?AbstractModel $subject): bool + { + if ($subject && method_exists($subject, 'registerVisibilityScoper')) { + return (bool) $subject->newQuery()->whereVisibleTo($user)->find($subject->id); + } + + return true; + } + /** * Adds a notification driver to the list. * diff --git a/framework/core/src/Post/Command/PostReply.php b/framework/core/src/Post/Command/PostReply.php index ae1d4c03c..226464926 100644 --- a/framework/core/src/Post/Command/PostReply.php +++ b/framework/core/src/Post/Command/PostReply.php @@ -41,17 +41,23 @@ class PostReply */ public $ipAddress; + /** + * @var bool + */ + public $isFirstPost; + /** * @param int $discussionId The ID of the discussion to post the reply to. * @param User $actor The user who is performing the action. * @param array $data The attributes to assign to the new post. * @param string $ipAddress The IP address of the actor. */ - public function __construct($discussionId, User $actor, array $data, $ipAddress = null) + public function __construct($discussionId, User $actor, array $data, $ipAddress = null, bool $isFirstPost = false) { $this->discussionId = $discussionId; $this->actor = $actor; $this->data = $data; $this->ipAddress = $ipAddress; + $this->isFirstPost = $isFirstPost; } } diff --git a/framework/core/src/Post/Command/PostReplyHandler.php b/framework/core/src/Post/Command/PostReplyHandler.php index b0bfb8877..f634a2dac 100644 --- a/framework/core/src/Post/Command/PostReplyHandler.php +++ b/framework/core/src/Post/Command/PostReplyHandler.php @@ -74,7 +74,7 @@ class PostReplyHandler // If this is the first post in the discussion, it's technically not a // "reply", so we won't check for that permission. - if ($discussion->first_post_id !== null) { + if (! $command->isFirstPost) { $actor->assertCan('reply', $discussion); } diff --git a/framework/core/src/Post/PostRepository.php b/framework/core/src/Post/PostRepository.php index 96ccba0b5..5d03ce062 100644 --- a/framework/core/src/Post/PostRepository.php +++ b/framework/core/src/Post/PostRepository.php @@ -29,7 +29,7 @@ class PostRepository * @param User|null $user * @return Builder */ - protected function queryVisibleTo(User $user = null) + public function queryVisibleTo(User $user = null) { $query = $this->query(); diff --git a/framework/core/tests/integration/api/posts/CreateTest.php b/framework/core/tests/integration/api/posts/CreateTest.php index a69d42783..41b61276d 100644 --- a/framework/core/tests/integration/api/posts/CreateTest.php +++ b/framework/core/tests/integration/api/posts/CreateTest.php @@ -10,6 +10,7 @@ namespace Flarum\Tests\integration\api\posts; use Carbon\Carbon; +use Flarum\Group\Group; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; @@ -26,36 +27,70 @@ class CreateTest extends TestCase $this->prepareDatabase([ 'discussions' => [ - ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2], + ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'first_post_id' => 1], + // Discussion with deleted first post. + ['id' => 2, 'title' => __CLASS__, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'first_post_id' => null], + ], + 'posts' => [ + ['id' => 1, 'discussion_id' => 1, 'number' => 1, 'created_at' => Carbon::now()->subDay()->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => ''], ], 'users' => [ $this->normalUser(), - ] + ['id' => 3, 'username' => 'restricted', 'email' => 'restricted@machine.local', 'is_email_confirmed' => 1], + ], + 'groups' => [ + ['id' => 40, 'name_singular' => 'tess', 'name_plural' => 'tess'], + ], + 'group_user' => [ + ['group_id' => 40, 'user_id' => 3], + ], + 'group_permission' => [ + ['group_id' => 40, 'permission' => 'discussion.reply'], + ], ]); } /** + * @dataProvider discussionRepliesPrvider * @test */ - public function can_create_reply() + public function can_create_reply_if_allowed(int $actorId, int $discussionId, int $responseStatus) { + // Reset permissions for normal users group. + $this->database() + ->table('group_permission') + ->where('permission', 'discussion.reply') + ->where('group_id', Group::MEMBER_ID) + ->delete(); + $response = $this->send( $this->request('POST', '/api/posts', [ - 'authenticatedAs' => 2, + 'authenticatedAs' => $actorId, 'json' => [ 'data' => [ 'attributes' => [ 'content' => 'reply with predetermined content for automated testing - too-obscure', ], 'relationships' => [ - 'discussion' => ['data' => ['id' => 1]], + 'discussion' => ['data' => ['id' => $discussionId]], ], ], ], ]) ); - $this->assertEquals(201, $response->getStatusCode()); + $this->assertEquals($responseStatus, $response->getStatusCode()); + } + + public function discussionRepliesPrvider(): array + { + return [ + // [$actorId, $discussionId, $responseStatus] + 'can_create_reply_with_ability' => [3, 1, 201], + 'cannot_create_reply_without_ability' => [2, 1, 403], + 'can_create_reply_with_ability_when_first_post_is_deleted' => [3, 2, 201], + 'cannot_create_reply_without_ability_when_first_post_is_deleted' => [2, 2, 403], + ]; } /** diff --git a/framework/core/tests/integration/notification/NotificationSyncerTest.php b/framework/core/tests/integration/notification/NotificationSyncerTest.php new file mode 100644 index 000000000..cbe9287ea --- /dev/null +++ b/framework/core/tests/integration/notification/NotificationSyncerTest.php @@ -0,0 +1,168 @@ +prepareDatabase([ + 'users' => [ + $this->normalUser(), + ['id' => 3, 'username' => 'Receiver', 'email' => 'receiver@machine.local', 'is_email_confirmed' => 1], + ], + 'discussions' => [ + ['id' => 1, 'title' => 'Public discussion', 'created_at' => Carbon::parse('2021-11-01 13:00:00')->toDateTimeString(), 'user_id' => 2, 'first_post_id' => 1, 'comment_count' => 2, 'is_private' => 0, 'last_post_number' => 2], + + ['id' => 2, 'title' => 'Private discussion', 'created_at' => Carbon::parse('2021-11-01 13:00:00')->toDateTimeString(), 'user_id' => 2, 'first_post_id' => 3, 'comment_count' => 2, 'is_private' => 1, 'last_post_number' => 2], + ], + 'posts' => [ + ['id' => 1, 'discussion_id' => 1, 'number' => 1, 'created_at' => Carbon::parse('2021-11-01 13:00:00')->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '', 'is_private' => 0], + ['id' => 2, 'discussion_id' => 1, 'number' => 2, 'created_at' => Carbon::parse('2021-11-01 13:00:03')->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '', 'is_private' => 1], + + ['id' => 3, 'discussion_id' => 2, 'number' => 1, 'created_at' => Carbon::parse('2021-11-01 13:00:00')->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '', 'is_private' => 0], + ], + ]); + } + + /** + * @dataProvider visibleSubjectsProvider + * @param class-string $subjectClass + * @test + */ + public function can_receive_notification_for_visible_subjects(string $subjectClass, int $subjectId, string $serializer) + { + $this->expect_notification_count_from_sending_notification_type_with_subject( + 2, + $subjectClass, + $subjectId, + $serializer + ); + } + + /** + * @dataProvider invisibleSubjectsProvider + * @test + */ + public function cannot_receive_notification_for_restricted_subjects(string $subjectClass, int $subjectId, string $serializer) + { + $this->expect_notification_count_from_sending_notification_type_with_subject( + 0, + $subjectClass, + $subjectId, + $serializer + ); + } + + /** + * @param class-string $subjectClass + */ + protected function expect_notification_count_from_sending_notification_type_with_subject(int $count, string $subjectClass, int $subjectId, string $serializer) + { + CustomNotificationType::$subjectModel = $subjectClass; + + $this->extend( + (new Extend\Notification()) + ->type(CustomNotificationType::class, $serializer, ['alert']) + ); + + /** @var NotificationSyncer $syncer */ + $syncer = $this->app()->getContainer()->make(NotificationSyncer::class); + + $subject = $subjectClass::query()->find($subjectId); + + $syncer->sync( + $blueprint = new CustomNotificationType($subject), + User::query() + ->whereIn('id', [1, 3]) + ->get() + ->all() + ); + + $this->assertEquals( + $count, + Notification::query() + ->matchingBlueprint($blueprint) + ->whereSubject($subject) + ->count() + ); + } + + public function visibleSubjectsProvider() + { + return [ + [Post::class, 1, BasicPostSerializer::class], + [Discussion::class, 1, BasicDiscussionSerializer::class], + ]; + } + + public function invisibleSubjectsProvider() + { + return [ + [Post::class, 2, BasicPostSerializer::class], + [Discussion::class, 2, BasicDiscussionSerializer::class], + [Post::class, 3, BasicPostSerializer::class], + ]; + } +} + +class CustomNotificationType implements BlueprintInterface +{ + protected $subject; + public static $subjectModel; + + public function __construct($subject) + { + $this->subject = $subject; + } + + public function getFromUser() + { + return null; + } + + public function getSubject() + { + return $this->subject; + } + + public function getData() + { + return []; + } + + public static function getType() + { + return 'customNotificationType'; + } + + public static function getSubjectModel() + { + return self::$subjectModel; + } +}