From 132fdea659754ef57b7e3eb4b28c957cca06c9c5 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 10 Jan 2023 12:43:46 +0100 Subject: [PATCH 01/11] Merge pull request from GHSA-22m9-m3ww-53h3 * fix: check post visibility when mentioning Signed-off-by: Sami Mazouz * fix: `mentionsPosts` include is not used and leaks private posts Signed-off-by: Sami Mazouz * chre: use `PostRepository` Signed-off-by: Sami Mazouz Signed-off-by: Sami Mazouz --- extensions/mentions/composer.json | 2 +- extensions/mentions/extend.php | 2 -- extensions/mentions/src/ConfigureMentions.php | 11 ++++-- .../integration/api/PostMentionsTest.php | 35 +++++++++++++++++++ framework/core/src/Formatter/Formatter.php | 7 ++++ framework/core/src/Post/PostRepository.php | 2 +- 6 files changed, 52 insertions(+), 7 deletions(-) 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..5b4c7ac71 100644 --- a/extensions/mentions/src/ConfigureMentions.php +++ b/extensions/mentions/src/ConfigureMentions.php @@ -12,10 +12,12 @@ 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; use s9e\TextFormatter\Configurator; +use s9e\TextFormatter\Parser; class ConfigureMentions { @@ -115,7 +117,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 +127,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/framework/core/src/Formatter/Formatter.php b/framework/core/src/Formatter/Formatter.php index 2db336cd2..24808a2f8 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/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(); From be63b28437f73a2e35b0ca83333b1f45a4c23096 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Tue, 10 Jan 2023 11:44:00 +0000 Subject: [PATCH 02/11] Apply fixes from StyleCI --- extensions/mentions/src/ConfigureMentions.php | 2 -- framework/core/src/Formatter/Formatter.php | 10 +++++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/extensions/mentions/src/ConfigureMentions.php b/extensions/mentions/src/ConfigureMentions.php index 5b4c7ac71..8378e3285 100644 --- a/extensions/mentions/src/ConfigureMentions.php +++ b/extensions/mentions/src/ConfigureMentions.php @@ -11,13 +11,11 @@ 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; use s9e\TextFormatter\Configurator; -use s9e\TextFormatter\Parser; class ConfigureMentions { diff --git a/framework/core/src/Formatter/Formatter.php b/framework/core/src/Formatter/Formatter.php index 24808a2f8..2176cfb69 100644 --- a/framework/core/src/Formatter/Formatter.php +++ b/framework/core/src/Formatter/Formatter.php @@ -91,11 +91,11 @@ 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. - */ + /* + * 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) { From a131e87911b6c0499c2f0472d985164f5ff0c3c4 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 10 Jan 2023 12:44:50 +0100 Subject: [PATCH 03/11] Merge pull request from GHSA-8gcg-vwmw-rxj4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: notifications grant access to private data of posts * chore: fix tests * test: start with tests about notification subject visibility Signed-off-by: Sami Mazouz * fix: check subject access before sending notification to user Signed-off-by: Sami Mazouz Signed-off-by: Sami Mazouz Co-authored-by: Daniël Klabbers --- .../src/Notification/NotificationSyncer.php | 19 ++ .../notification/NotificationSyncerTest.php | 169 ++++++++++++++++++ 2 files changed, 188 insertions(+) create mode 100644 framework/core/tests/integration/notification/NotificationSyncerTest.php 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/tests/integration/notification/NotificationSyncerTest.php b/framework/core/tests/integration/notification/NotificationSyncerTest.php new file mode 100644 index 000000000..635a93770 --- /dev/null +++ b/framework/core/tests/integration/notification/NotificationSyncerTest.php @@ -0,0 +1,169 @@ +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() + ); + } + + protected function visibleSubjectsProvider() + { + return [ + [Post::class, 1, BasicPostSerializer::class], + [Discussion::class, 1, BasicDiscussionSerializer::class], + ]; + } + + protected 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; + } +} From 248a71d9b53b059652126535698c3cebd8a874a3 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Tue, 10 Jan 2023 11:44:58 +0000 Subject: [PATCH 04/11] Apply fixes from StyleCI --- .../tests/integration/notification/NotificationSyncerTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/framework/core/tests/integration/notification/NotificationSyncerTest.php b/framework/core/tests/integration/notification/NotificationSyncerTest.php index 635a93770..c61295571 100644 --- a/framework/core/tests/integration/notification/NotificationSyncerTest.php +++ b/framework/core/tests/integration/notification/NotificationSyncerTest.php @@ -65,7 +65,6 @@ class NotificationSyncerTest extends TestCase ); } - /** * @dataProvider invisibleSubjectsProvider * @test From 12dfcc5c7960e8dc1e35e9d0a10d071923b39a76 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 10 Jan 2023 12:45:29 +0100 Subject: [PATCH 05/11] Merge pull request from GHSA-hph3-hv3c-7725 * test: add reply creation tests Signed-off-by: Sami Mazouz * fix: access checking being bypassed for post creation when first post is deleted Signed-off-by: Sami Mazouz * chore: recover tests Signed-off-by: Sami Mazouz * chore: make provider public Signed-off-by: Sami Mazouz Signed-off-by: Sami Mazouz --- .../Command/StartDiscussionHandler.php | 2 +- framework/core/src/Post/Command/PostReply.php | 8 +++- .../src/Post/Command/PostReplyHandler.php | 2 +- .../integration/api/posts/CreateTest.php | 47 ++++++++++++++++--- 4 files changed, 50 insertions(+), 9 deletions(-) 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/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/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], + ]; } /** From 666223fa8c11ac10e670b8fd21db87e532f4af6e Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 10 Jan 2023 13:06:16 +0100 Subject: [PATCH 06/11] test: make data providers public Signed-off-by: Sami Mazouz --- .../tests/integration/notification/NotificationSyncerTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/framework/core/tests/integration/notification/NotificationSyncerTest.php b/framework/core/tests/integration/notification/NotificationSyncerTest.php index c61295571..cbe9287ea 100644 --- a/framework/core/tests/integration/notification/NotificationSyncerTest.php +++ b/framework/core/tests/integration/notification/NotificationSyncerTest.php @@ -113,7 +113,7 @@ class NotificationSyncerTest extends TestCase ); } - protected function visibleSubjectsProvider() + public function visibleSubjectsProvider() { return [ [Post::class, 1, BasicPostSerializer::class], @@ -121,7 +121,7 @@ class NotificationSyncerTest extends TestCase ]; } - protected function invisibleSubjectsProvider() + public function invisibleSubjectsProvider() { return [ [Post::class, 2, BasicPostSerializer::class], From 02556c6ca6c4f5c5148252517ec93beded97f021 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 10 Jan 2023 13:17:54 +0100 Subject: [PATCH 07/11] chore: prepare `v1.6.3` release Signed-off-by: Sami Mazouz --- CHANGELOG.md | 6 ++++++ framework/core/src/Foundation/Application.php | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9d64227d..75641d074 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # 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). + ## [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/framework/core/src/Foundation/Application.php b/framework/core/src/Foundation/Application.php index fc1978516..3885bb7b4 100644 --- a/framework/core/src/Foundation/Application.php +++ b/framework/core/src/Foundation/Application.php @@ -21,7 +21,7 @@ class Application * * @var string */ - const VERSION = '1.6.2'; + const VERSION = '1.6.3'; /** * The IoC container for the Flarum application. From e5f05166a062a9a6eb7c12e28728bfd5db7270e3 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 10 Jan 2023 15:04:02 +0100 Subject: [PATCH 08/11] fix(subscriptions): post notifications not getting access checked Signed-off-by: Sami Mazouz --- extensions/subscriptions/extend.php | 4 +- .../FilterVisiblePostsBeforeSending.php | 28 ++++++++++++ .../api/discussions/ReplyNotificationTest.php | 45 +++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 extensions/subscriptions/src/Notification/FilterVisiblePostsBeforeSending.php 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..818c8e9d5 --- /dev/null +++ b/extensions/subscriptions/src/Notification/FilterVisiblePostsBeforeSending.php @@ -0,0 +1,28 @@ +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()); + } } From c8d9f1111ede8d21317236b7fe7da245873f2312 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Tue, 10 Jan 2023 14:04:18 +0000 Subject: [PATCH 09/11] Apply fixes from StyleCI --- .../src/Notification/FilterVisiblePostsBeforeSending.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/extensions/subscriptions/src/Notification/FilterVisiblePostsBeforeSending.php b/extensions/subscriptions/src/Notification/FilterVisiblePostsBeforeSending.php index 818c8e9d5..b913d9193 100644 --- a/extensions/subscriptions/src/Notification/FilterVisiblePostsBeforeSending.php +++ b/extensions/subscriptions/src/Notification/FilterVisiblePostsBeforeSending.php @@ -1,5 +1,12 @@ Date: Tue, 10 Jan 2023 15:06:03 +0100 Subject: [PATCH 10/11] chore(subscriptions): prepare Signed-off-by: Sami Mazouz --- extensions/subscriptions/composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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": { From 243bc139b0fc6aea7526bf8e58e9d618920bb80c Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Tue, 10 Jan 2023 15:22:17 +0100 Subject: [PATCH 11/11] chore: changelog Signed-off-by: Sami Mazouz --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75641d074..8bb105bd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ * 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