chore: Merge 1.6.3 into main

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).

extensions/mentions/composer.json

@@ -19,7 +19,7 @@
         }
     ],
     "require": {
-        "flarum/core": "^1.6"
+        "flarum/core": "^1.6.3"
     },
     "autoload": {
         "psr-4": {

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))

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@["|“](?<displayname>((?!"#[a-z]{0,3}[0-9]+).)+)["|”]#p(?<id>[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);

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' => '<r><POSTMENTION displayname="TobyFlarum___" id="5" number="2" discussionid="2" username="toby">@tobyuuu#5</POSTMENTION></r>'],
@@ -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' => '<r><POSTMENTION displayname="Bad &quot;#p6 User" id="9" number="10" discussionid="2">@"Bad "#p6 User"#p9</POSTMENTION></r>'],
                 ['id' => 11, 'number' => 12, 'discussion_id' => 2, 'created_at' => Carbon::now(), 'user_id' => 40, 'type' => 'comment', 'content' => '<r><POSTMENTION displayname="Bad &quot;#p6 User" id="9" number="10" discussionid="2">@"Bad "#p6 User"#p9</POSTMENTION></r>'],
                 ['id' => 12, 'number' => 13, 'discussion_id' => 2, 'created_at' => Carbon::now(), 'user_id' => 4, 'type' => 'comment', 'content' => '<r><POSTMENTION displayname="deleted_user" id="11" number="12" discussionid="2">@"acme"#p11</POSTMENTION></r>'],
+
+                // Restricted access
+                ['id' => 50, 'number' => 1, 'discussion_id' => 50, 'created_at' => Carbon::now(), 'user_id' => 3, 'type' => 'comment', 'content' => '<r>no</r>'],
             ],
             '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
      */

extensions/subscriptions/composer.json

@@ -19,7 +19,7 @@
         }
     ],
     "require": {
-        "flarum/core": "^1.6"
+        "flarum/core": "^1.6.3"
     },
     "autoload": {
         "psr-4": {

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) {

extensions/subscriptions/src/Notification/FilterVisiblePostsBeforeSending.php

@@ -0,0 +1,35 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Subscriptions\Notification;
+
+use Flarum\Notification\Blueprint\BlueprintInterface;
+
+class FilterVisiblePostsBeforeSending
+{
+    public function __invoke(BlueprintInterface $blueprint, array $recipients): array
+    {
+        if ($blueprint instanceof NewPostBlueprint) {
+            $newRecipients = [];
+
+            // Flarum has built-in access control for the notification subject,
+            // but subscriptions post notifications has the discussion as the subject.
+            // We'll add a post visibility check so that users can't get access to hidden replies by subscribing.
+            foreach ($recipients as $recipient) {
+                if ($blueprint->post->isVisibleTo($recipient)) {
+                    $newRecipients[] = $recipient;
+                }
+            }
+
+            return $newRecipients;
+        }
+
+        return $recipients;
+    }
+}

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());
+    }
 }

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();

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);
         }

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.
      *

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;
     }
 }

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);
         }
 

framework/core/src/Post/PostRepository.php

@@ -29,7 +29,7 @@ class PostRepository
      * @param User|null $user
      * @return Builder<Post>
      */
-    protected function queryVisibleTo(User $user = null)
+    public function queryVisibleTo(User $user = null)
     {
         $query = $this->query();
 

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' => '<t></t>'],
             ],
             '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],
+        ];
     }
 
     /**

framework/core/tests/integration/notification/NotificationSyncerTest.php

@@ -0,0 +1,168 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tests\integration\notification;
+
+use Carbon\Carbon;
+use Flarum\Api\Serializer\BasicDiscussionSerializer;
+use Flarum\Api\Serializer\BasicPostSerializer;
+use Flarum\Database\AbstractModel;
+use Flarum\Discussion\Discussion;
+use Flarum\Extend;
+use Flarum\Notification\Blueprint\BlueprintInterface;
+use Flarum\Notification\Notification;
+use Flarum\Notification\NotificationSyncer;
+use Flarum\Post\Post;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Flarum\User\User;
+
+class NotificationSyncerTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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' => '<t></t>', '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' => '<t></t>', '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' => '<t></t>', 'is_private' => 0],
+            ],
+        ]);
+    }
+
+    /**
+     * @dataProvider visibleSubjectsProvider
+     * @param class-string<AbstractModel> $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<AbstractModel> $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;
+    }
+}