Address code-review findings (#1 hardening, #2, #6, #7, #8)

js/forum.js

@@ -10,8 +10,6 @@ var qrModal = require('./qrModal');
     var UserPage = flarum.reg.get('core', 'forum/components/UserPage');
     var LinkButton = flarum.reg.get('core', 'common/components/LinkButton');
     var Link = flarum.reg.get('core', 'common/components/Link');
-    var Avatar = flarum.reg.get('core', 'common/components/Avatar');
-    var humanTime = flarum.reg.get('core', 'common/helpers/humanTime');
     var LoadingIndicator = flarum.reg.get('core', 'common/components/LoadingIndicator');
     var UserPageResolver = flarum.reg.get('core', 'forum/resolvers/UserPageResolver');
 
@@ -307,71 +305,91 @@ var qrModal = require('./qrModal');
       });
     });
 
-    // Core's NotificationList groups notifications by discussion and lumps
-    // anything not tied to one (like this type) into a neutral group labelled
-    // with the forum title. There's no per-type hook, so content() is
-    // reimplemented to route referral notifications into their own
-    // "Referrals" group while leaving all other grouping untouched.
+    // Core's NotificationList.content groups notifications by discussion and
+    // lumps anything not tied to one (like this type) into a neutral group
+    // labelled with the forum title. Core exposes no per-type or per-group
+    // hook, so content() is reimplemented here to route referral notifications
+    // into their own "Referrals" group while mirroring core's grouping for
+    // everything else.
+    //
+    // Fully overriding a core render method is inherently coupled to core's
+    // internals, so two guards keep it safe:
+    //   1. The logic below mirrors core's NotificationList.content as of
+    //      Flarum 2.0.0-rc.5 (flarum/core: forum/components/NotificationList).
+    //      When bumping core, re-check that method and reconcile this copy.
+    //   2. The whole body runs inside a try/catch that falls back to core's
+    //      original content() on any error, so a future core refactor degrades
+    //      to plain (un-relabelled) notification rendering rather than breaking
+    //      the entire notifications page. RegistrationReferralTest locks the
+    //      serialized contract this override reads (contentType + subject).
     function installNotificationGrouping(NotificationList) {
       if (!NotificationList || NotificationList._referralExtended) return;
       NotificationList._referralExtended = true;
 
       override(NotificationList.prototype, 'content', function (original, state) {
-        if (state.isLoading() || !state.hasItems()) return null;
+        try {
+          if (state.isLoading() || !state.hasItems()) return null;
 
-        var HeaderListGroup = flarum.reg.get('core', 'forum/components/HeaderListGroup');
-        var NotificationType = flarum.reg.get('core', 'forum/components/NotificationType');
-        var Discussion = flarum.reg.get('core', 'common/models/Discussion');
-        var listItems = flarum.reg.get('core', 'common/helpers/listItems');
+          var HeaderListGroup = flarum.reg.get('core', 'forum/components/HeaderListGroup');
+          var NotificationType = flarum.reg.get('core', 'forum/components/NotificationType');
+          var Discussion = flarum.reg.get('core', 'common/models/Discussion');
+          var listItems = flarum.reg.get('core', 'common/helpers/listItems');
 
-        return state.getPages().map(function (page) {
-          var groups = [];
-          var byKey = {};
+          return state.getPages().map(function (page) {
+            var groups = [];
+            var byKey = {};
 
-          page.items.forEach(function (notification) {
-            var subject = notification.subject();
-            if (typeof subject === 'undefined') return;
+            page.items.forEach(function (notification) {
+              var subject = notification.subject();
+              if (typeof subject === 'undefined') return;
 
-            var contentType = notification.contentType && notification.contentType();
-            var isReferral = contentType === NOTIFICATION_TYPE;
+              var contentType = notification.contentType && notification.contentType();
+              var isReferral = contentType === NOTIFICATION_TYPE;
 
-            // Mirror core's discussion resolution for everything else.
-            var discussion = null;
-            if (!isReferral) {
-              if (Discussion && subject instanceof Discussion) discussion = subject;
-              else if (subject && subject.discussion) discussion = subject.discussion();
-            }
+              // Mirror core's discussion resolution for everything else.
+              var discussion = null;
+              if (!isReferral) {
+                if (Discussion && subject instanceof Discussion) discussion = subject;
+                else if (subject && subject.discussion) discussion = subject.discussion();
+              }
 
-            var key = isReferral ? 'linkrobins-referral' : discussion ? 'd' + discussion.id() : 'neutral';
+              var key = isReferral ? 'linkrobins-referral' : discussion ? 'd' + discussion.id() : 'neutral';
 
-            byKey[key] = byKey[key] || { discussion: discussion, referral: isReferral, notifications: [] };
-            byKey[key].notifications.push(notification);
-            if (groups.indexOf(byKey[key]) === -1) groups.push(byKey[key]);
-          });
+              byKey[key] = byKey[key] || { discussion: discussion, referral: isReferral, notifications: [] };
+              byKey[key].notifications.push(notification);
+              if (groups.indexOf(byKey[key]) === -1) groups.push(byKey[key]);
+            });
 
-          return groups.map(function (group) {
-            var label;
-            if (group.referral) {
-              label = app.translator.trans('linkrobins-referral.forum.notifications.group_label');
-            } else if (group.discussion) {
-              var badges = group.discussion.badges().toArray();
-              label = m(Link, { href: app.route.discussion(group.discussion) }, [
-                badges && badges.length ? m('ul', { className: 'HeaderListGroup-badges badges' }, listItems(badges)) : null,
-                m('span', null, group.discussion.title()),
-              ]);
-            } else {
-              label = app.forum.attribute('title');
-            }
-
-            return m(
-              HeaderListGroup,
-              { label: label },
-              group.notifications.map(function (notification) {
-                return m(NotificationType, { notification: notification });
-              })
-            );
+            return groups.map(function (group) {
+              var label;
+              if (group.referral) {
+                label = app.translator.trans('linkrobins-referral.forum.notifications.group_label');
+              } else if (group.discussion) {
+                var badges = group.discussion.badges().toArray();
+                label = m(Link, { href: app.route.discussion(group.discussion) }, [
+                  badges && badges.length ? m('ul', { className: 'HeaderListGroup-badges badges' }, listItems(badges)) : null,
+                  m('span', null, group.discussion.title()),
+                ]);
+              } else {
+                label = app.forum.attribute('title');
+              }
+
+              return m(
+                HeaderListGroup,
+                { label: label },
+                group.notifications
+                  .map(function (notification) {
+                    return m(NotificationType, { notification: notification });
+                  })
+                  .filter(Boolean)
+              );
+            });
           });
-        });
+        } catch (e) {
+          // A core-internal change broke the reproduced pipeline; render core's
+          // own grouping so notifications still show (without the referral group).
+          return original(state);
+        }
       });
     }
 

src/Api/UserResourceFields.php

@@ -12,6 +12,16 @@
 
 class UserResourceFields
 {
+    /**
+     * Upper bound on how many referred users a single ?include=referredUsers
+     * loads. The relationship is only exposed to the user themselves or an
+     * admin, but a prolific referrer could still have thousands of rows; an
+     * uncapped ->get() would pull the whole set into memory and emit a huge
+     * JSON:API payload. The forum UI only renders the referral *count*, so
+     * this cap is purely a guard for direct API consumers.
+     */
+    protected const REFERRED_USERS_LIMIT = 100;
+
     public function __construct(
         protected EligibilityChecker $eligibility
     ) {}
@@ -61,11 +71,16 @@ public function __invoke(): array
                 ->includable()
                 ->visible(fn (User $user, $context) => $context->getActor()->id === $user->id || $context->getActor()->isAdmin())
                 ->get(function (User $user) {
+                    // Capped and ordered newest-first (see REFERRED_USERS_LIMIT).
                     return ReferralRelation::where('referred_by_user_id', $user->id)
                         ->with('user')
+                        ->orderByDesc('id')
+                        ->limit(self::REFERRED_USERS_LIMIT)
                         ->get()
                         ->map(fn ($r) => $r->user)
-                        ->filter();
+                        ->filter()
+                        ->values()
+                        ->all();
                 }),
         ];
     }

src/InviteCode.php

@@ -54,11 +54,17 @@ public static function generateCode(): string
 
     protected static function generateUniqueCode(): string
     {
-        $code = static::generateCode();
-        while (static::where('code', $code)->exists()) {
+        // Collisions are astronomically unlikely (32^8 ~ 1.1e12 codes), but
+        // cap the retries so a saturated table or a narrowed alphabet surfaces
+        // as a clear 500 in the logs rather than a hung worker looping forever.
+        for ($attempt = 0; $attempt < 10; $attempt++) {
             $code = static::generateCode();
+            if (! static::where('code', $code)->exists()) {
+                return $code;
+            }
         }
-        return $code;
+
+        throw new \RuntimeException('Unable to generate a unique referral code after 10 attempts.');
     }
 
     public static function getOrCreateForUser(User $user): self

src/RecordReferral.php

@@ -13,6 +13,10 @@ class RecordReferral
 {
     public function __construct(
         protected LoggerInterface $logger,
+        protected NotificationSyncer $notifications,
+        // Retained only to resolve the request-scoped PendingReferralState
+        // fresh per request (a constructor-injected singleton would leak state
+        // across registrations under persistent runtimes).
         protected Container $container
     ) {}
 
@@ -55,7 +59,7 @@ public function handle(Registered $event): void
                     // Tell the referrer their code was used. Guarded by the
                     // exists() check above, so a duplicate Registered event
                     // can't double-notify.
-                    $this->container->make(NotificationSyncer::class)->sync(
+                    $this->notifications->sync(
                         new ReferralRegisteredBlueprint($user),
                         [$referrer]
                     );

tests/integration/api/ReferredUsersIncludeTest.php

@@ -0,0 +1,81 @@
+<?php
+
+/*
+ * This file is part of linkrobins/referral.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace LinkRobins\Referral\Tests\integration\api;
+
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use PHPUnit\Framework\Attributes\Test;
+
+class ReferredUsersIncludeTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        $this->extension('linkrobins-referral');
+
+        $this->prepareDatabase([
+            'users' => [
+                $this->normalUser(), // id 2, the referrer
+                ['id' => 3, 'username' => 'invitee', 'password' => '$2y$10$LO59tiT7uggl6Oe23o/O6.utnF6ipngYjvMvaxo1TciKqBttDNKim', 'email' => 'invitee@machine.local', 'is_email_confirmed' => 1, 'referral_count' => 0],
+            ],
+            'referral_invited_user' => [
+                ['user_id' => 3, 'referred_by_user_id' => 2],
+            ],
+        ]);
+
+        // The referrer's denormalised counter.
+        $this->database()->table('users')->where('id', 2)->update(['referral_count' => 1]);
+    }
+
+    #[Test]
+    public function referred_users_relationship_serialises_for_the_owner(): void
+    {
+        $response = $this->send(
+            $this->request('GET', '/api/users/2', ['authenticatedAs' => 2])
+                ->withQueryParams(['include' => 'referredUsers'])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode(), (string) $response->getBody());
+
+        $body = json_decode((string) $response->getBody(), true);
+
+        $linkage = array_column($body['data']['relationships']['referredUsers']['data'] ?? [], 'id');
+        $this->assertContains('3', $linkage, 'The referred user should appear in the relationship linkage.');
+
+        $includedUserIds = array_column(
+            array_filter($body['included'] ?? [], fn ($r) => $r['type'] === 'users'),
+            'id'
+        );
+        $this->assertContains('3', $includedUserIds, 'The referred user should be included in the response.');
+    }
+
+    #[Test]
+    public function referred_users_relationship_is_hidden_from_other_users(): void
+    {
+        $response = $this->send(
+            $this->request('GET', '/api/users/2', ['authenticatedAs' => 3])
+                ->withQueryParams(['include' => 'referredUsers'])
+        );
+
+        // Not the owner and not an admin: the relationship is not exposed, and
+        // the request must still succeed rather than error.
+        $this->assertEquals(200, $response->getStatusCode(), (string) $response->getBody());
+
+        $body = json_decode((string) $response->getBody(), true);
+        $includedUserIds = array_column(
+            array_filter($body['included'] ?? [], fn ($r) => $r['type'] === 'users'),
+            'id'
+        );
+        $this->assertNotContains('3', $includedUserIds);
+    }
+}

tests/integration/api/RegistrationReferralTest.php

@@ -108,6 +108,55 @@ public function a_recorded_referral_notifies_the_referrer(): void
         $this->assertEquals($newUser->id, $notification->from_user_id);
     }
 
+    /**
+     * The forum-side NotificationList.content override (js/forum.js) fully
+     * reimplements core's grouping so referral notifications land in their own
+     * group instead of the neutral "forum title" bucket. That reproduced logic
+     * keys on two things in the serialized notification: contentType ===
+     * 'linkrobinsReferralRegistered', and a subject that is a user (never a
+     * discussion). This test locks that contract at the API layer so a change
+     * that would silently break the frontend grouping fails loudly here.
+     */
+    #[Test]
+    public function the_referral_notification_serialises_with_the_shape_the_grouping_relies_on(): void
+    {
+        $this->prepareDatabase([
+            'referral_invite_codes' => [
+                ['id' => 1, 'user_id' => 2, 'code' => 'TESTCODE', 'uses' => 0],
+            ],
+        ]);
+
+        $this->assertEquals(201, $this->register('TESTCODE')->getStatusCode());
+
+        // Explicit include of just the subject: this endpoint's default include
+        // pulls in subject.discussion, which core only makes valid once some
+        // registered notification subject type is discussion-bearing (a Post),
+        // so a plain GET here in a minimal core+referral forum is a separate
+        // concern. This test only cares about the serialized shape the frontend
+        // grouping reads, so it requests the subject relationship directly. The
+        // query param must go through withQueryParams(): the test request helper
+        // doesn't parse the query string off the URI.
+        $response = $this->send(
+            $this->request('GET', '/api/notifications', ['authenticatedAs' => 2])
+                ->withQueryParams(['include' => 'fromUser,subject'])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+        $body = json_decode($response->getBody()->getContents(), true);
+
+        $referral = null;
+        foreach ($body['data'] as $notification) {
+            if (($notification['attributes']['contentType'] ?? null) === 'linkrobinsReferralRegistered') {
+                $referral = $notification;
+                break;
+            }
+        }
+
+        $this->assertNotNull($referral, 'Expected a notification with contentType linkrobinsReferralRegistered.');
+        // The subject the grouping inspects must be a user, not a discussion.
+        $this->assertEquals('users', $referral['relationships']['subject']['data']['type'] ?? null);
+    }
+
     #[Test]
     public function campaign_codes_do_not_notify_anyone(): void
     {