Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ and this project adheres to
### Added

- ✨(backend) add a is_first_connection flag to the User model#1938
### Changed

- ✨(backend) add limit on distinct reactions per comment #1978
Comment thread
maboukerfa marked this conversation as resolved.

## [v4.7.0] - 2026-03-09

Expand Down
29 changes: 27 additions & 2 deletions src/backend/core/api/viewsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -2625,6 +2625,7 @@ def get(self, request):
"POSTHOG_KEY",
"LANGUAGES",
"LANGUAGE_CODE",
"REACTIONS_MAX_PER_COMMENT",
"SENTRY_DSN",
"TRASHBIN_CUTOFF_DAYS",
]
Expand Down Expand Up @@ -2742,7 +2743,11 @@ class CommentViewSet(
permission_classes = [permissions.CommentPermission]
pagination_class = Pagination
serializer_class = serializers.CommentSerializer
queryset = models.Comment.objects.select_related("user").all()
queryset = (
models.Comment.objects.select_related("user")
.prefetch_related("reactions__users")
.all()
)

def get_queryset(self):
"""Override to filter on related resource."""
Expand Down Expand Up @@ -2776,9 +2781,29 @@ def reactions(self, request, *args, **kwargs):
serializer.is_valid(raise_exception=True)

if request.method == "POST":
emoji = serializer.validated_data["emoji"]

if (
not models.Reaction.objects.filter(
comment=comment, emoji=emoji
).exists()
and comment.reactions.count() >= settings.REACTIONS_MAX_PER_COMMENT
Comment thread
lunika marked this conversation as resolved.
):
return drf.response.Response(
{
"emoji": [
_(
"A comment can have a maximum of %(max)d distinct reactions."
)
% {"max": settings.REACTIONS_MAX_PER_COMMENT}
]
},
status=status.HTTP_400_BAD_REQUEST,
)

reaction, created = models.Reaction.objects.get_or_create(
comment=comment,
emoji=serializer.validated_data["emoji"],
emoji=emoji,
)
Comment on lines 2783 to 2807
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Serialize the limit check with the create step.

This is still a TOCTOU race: two concurrent POSTs with different new emojis can both observe count() < REACTIONS_MAX_PER_COMMENT and both create, leaving the comment above the configured cap. Since the backend limit is the main point of this PR, the POST path needs a transaction + row lock on the parent comment before the existence/count/create sequence.

Suggested fix
         if request.method == "POST":
             emoji = serializer.validated_data["emoji"]
-
-            if (
-                not models.Reaction.objects.filter(
-                    comment=comment, emoji=emoji
-                ).exists()
-                and comment.reactions.count() >= settings.REACTIONS_MAX_PER_COMMENT
-            ):
-                return drf.response.Response(
-                    {
-                        "emoji": [
-                            _(
-                                "A comment can have a maximum of %(max)d distinct reactions."
-                            )
-                            % {"max": settings.REACTIONS_MAX_PER_COMMENT}
-                        ]
-                    },
-                    status=status.HTTP_400_BAD_REQUEST,
-                )
-
-            reaction, created = models.Reaction.objects.get_or_create(
-                comment=comment,
-                emoji=emoji,
-            )
+            with transaction.atomic():
+                comment = models.Comment.objects.select_for_update().get(pk=comment.pk)
+                reactions = models.Reaction.objects.filter(comment=comment)
+
+                if (
+                    not reactions.filter(emoji=emoji).exists()
+                    and reactions.count() >= settings.REACTIONS_MAX_PER_COMMENT
+                ):
+                    return drf.response.Response(
+                        {
+                            "emoji": [
+                                _(
+                                    "A comment can have a maximum of %(max)d distinct reactions."
+                                )
+                                % {"max": settings.REACTIONS_MAX_PER_COMMENT}
+                            ]
+                        },
+                        status=status.HTTP_400_BAD_REQUEST,
+                    )
+
+                reaction, created = models.Reaction.objects.get_or_create(
+                    comment=comment,
+                    emoji=emoji,
+                )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/api/viewsets.py` around lines 2781 - 2805, The POST path is
vulnerable to TOCTOU: acquire a DB transaction and lock the parent comment row
with select_for_update before re-checking reactions and creating the Reaction;
specifically wrap the logic around the existing comment variable in an atomic
transaction (using Django's transaction.atomic()) and re-fetch the comment with
Comment.objects.select_for_update().get(pk=comment.pk), then perform the
existence check (models.Reaction.objects.filter(comment=comment,
emoji=emoji).exists()), re-evaluate comment.reactions.count() against
settings.REACTIONS_MAX_PER_COMMENT, and only then call
models.Reaction.objects.get_or_create(...) so the count/enforce-check and create
are done under the same lock.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunika, this is prone to race condition, a user can create a MAX+1 emoji if he spams emojis, is it worth fixing or it's an overkill ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's overkill and I'm not ok to add a lock on the table.

if not created and reaction.users.filter(id=request.user.id).exists():
return drf.response.Response(
Expand Down
5 changes: 5 additions & 0 deletions src/backend/core/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@ class Meta:
comment = factory.SubFactory(CommentFactory)
emoji = "test"

@classmethod
def generate_emojis(cls, n=10):
"""Generate a list of n unique emojis."""
return [fake.unique.emoji() for _ in range(n)]

@factory.post_generation
def users(self, create, extracted, **kwargs):
"""Add users to reaction from a given list of users or create one if not provided."""
Expand Down
53 changes: 53 additions & 0 deletions src/backend/core/tests/documents/test_api_documents_comments.py
Original file line number Diff line number Diff line change
Expand Up @@ -876,3 +876,56 @@ def test_delete_reaction_owned_by_the_current_user():

reaction.refresh_from_db()
assert reaction.users.exists()


def test_create_reaction_exceeds_maximum(settings):
"""
Users should not be able to add more than REACTIONS_MAX_PER_COMMENT
(here we set it to 10) distinct emoji reactions to a comment.
Comment thread
lunika marked this conversation as resolved.
They should, however, be able to add themselves to an existing reaction.
"""
user1 = factories.UserFactory()
user2 = factories.UserFactory()
document = factories.DocumentFactory(
link_reach="restricted",
users=[(user1, models.RoleChoices.ADMIN), (user2, models.RoleChoices.ADMIN)],
)
thread = factories.ThreadFactory(document=document)
comment = factories.CommentFactory(thread=thread)

client = APIClient()
client.force_login(user1)

# Add max distinct reactions
max_reactions = settings.REACTIONS_MAX_PER_COMMENT
Comment on lines +881 to +900
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Docstring/behavior mismatch: test doesn't actually set the limit to 10.

The docstring says "here we set it to 10" but the test reads settings.REACTIONS_MAX_PER_COMMENT without overriding it, so it runs against the default (15) and issues ~15 extra POSTs. Either explicitly set it to a small value via the settings fixture (which is also the point of using that fixture), or update the docstring.

🛠️ Proposed fix
 def test_create_reaction_exceeds_maximum(settings):
     """
     Users should not be able to add more than REACTIONS_MAX_PER_COMMENT
     (here we set it to 10) distinct emoji reactions to a comment.
     They should, however, be able to add themselves to an existing reaction.
     """
+    settings.REACTIONS_MAX_PER_COMMENT = 10
     user1 = factories.UserFactory()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/tests/documents/test_api_documents_comments.py` around lines
881 - 900, The test test_create_reaction_exceeds_maximum claims "here we set it
to 10" but never overrides settings.REACTIONS_MAX_PER_COMMENT; update the test
to explicitly set the limit using the provided settings fixture (e.g. assign
settings.REACTIONS_MAX_PER_COMMENT = 10 before creating reactions) so the test
adds exactly 10 distinct reactions and then verifies behavior, or alternatively
change the docstring to reflect that it uses the default setting; modify the
test logic in test_create_reaction_exceeds_maximum to reference the overridden
value consistently.

emojis = factories.ReactionFactory.generate_emojis(max_reactions + 1)
for emoji in emojis[:max_reactions]:
response = client.post(
f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/"
f"comments/{comment.id!s}/reactions/",
{"emoji": emoji},
)
assert response.status_code == 201

# Attempt to add another distinct reaction
response = client.post(
f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/"
f"comments/{comment.id!s}/reactions/",
{"emoji": emojis[max_reactions]},
)
assert response.status_code == 400
expected_message = (
f"A comment can have a maximum of {max_reactions} distinct reactions."
)
assert response.json() == {"emoji": [expected_message]}

# Attempt to add user2 to one of the existing reactions (should succeed)
client.force_login(user2)
response = client.post(
f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/"
f"comments/{comment.id!s}/reactions/",
{"emoji": emojis[0]},
)
assert response.status_code == 201
reaction = models.Reaction.objects.get(comment=comment, emoji=emojis[0])
assert reaction.users.count() == 2
6 changes: 6 additions & 0 deletions src/backend/impress/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,12 @@ class Base(Configuration):
environ_prefix=None,
)

REACTIONS_MAX_PER_COMMENT = values.IntegerValue(
15,
environ_name="REACTIONS_MAX_PER_COMMENT",
environ_prefix=None,
)
Comment thread
maboukerfa marked this conversation as resolved.

DOCUMENT_UNSAFE_MIME_TYPES = [
# Executable Files
"application/x-msdownload",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export interface ConfigResponse {
MEDIA_BASE_URL?: string;
POSTHOG_KEY?: PostHogConf;
SENTRY_DSN?: string;
REACTIONS_MAX_PER_COMMENT: number;
TRASHBIN_CUTOFF_DAYS?: number;
theme_customization?: ThemeCustomization;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export class DocsThreadStoreAuth extends ThreadStoreAuth {
constructor(
private readonly userId: string,
public canSee: boolean,
private readonly maxReactions: number,
) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
super();
}
Expand Down Expand Up @@ -68,13 +69,27 @@ export class DocsThreadStoreAuth extends ThreadStoreAuth {
}

if (!emoji) {
return true;
return comment.reactions.length < this.maxReactions;
}

return !comment.reactions.some(
const hasReactedWithEmoji = comment.reactions.some(
(reaction) =>
reaction.emoji === emoji && reaction.userIds.includes(this.userId),
);

if (hasReactedWithEmoji) {
return false;
}

const reactionExists = comment.reactions.some(
(reaction) => reaction.emoji === emoji,
);

if (reactionExists) {
return true;
}

return comment.reactions.length < this.maxReactions;
}

canDeleteReaction(comment: ClientCommentData, emoji?: string): boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { useTranslation } from 'react-i18next';
import { useCunninghamTheme } from '@/cunningham';
import { User, avatarUrlFromName } from '@/features/auth';
import { Doc, useProviderStore } from '@/features/docs/doc-management';
import { useConfig } from '@/core';

import { DocsThreadStore } from './DocsThreadStore';
import { DocsThreadStoreAuth } from './DocsThreadStoreAuth';
Expand All @@ -16,6 +17,7 @@ export function useComments(
const { provider } = useProviderStore();
const { t } = useTranslation();
const { themeTokens } = useCunninghamTheme();
const { data: config } = useConfig();

const threadStore = useMemo(() => {
return new DocsThreadStore(
Expand All @@ -24,9 +26,16 @@ export function useComments(
new DocsThreadStoreAuth(
encodeURIComponent(user?.full_name || ''),
canComment,
config?.REACTIONS_MAX_PER_COMMENT ?? 0,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Minor: ?? 0 disables all new reactions until config loads.

When config hasn't resolved yet (first load) or cached config predates this field, REACTIONS_MAX_PER_COMMENT is undefined and this falls back to 0, which gates canAddReaction(..., undefined) to false and blocks any new distinct emoji. This is briefly user-visible because useQuery seeds initialData from localStorage before the refetch completes.

Consider using the backend's documented default (15) as the fallback, or rendering the picker as loading until config is defined:

💡 Optional refactor
-        config?.REACTIONS_MAX_PER_COMMENT ?? 0,
+        config?.REACTIONS_MAX_PER_COMMENT ?? 15,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
config?.REACTIONS_MAX_PER_COMMENT ?? 0,
config?.REACTIONS_MAX_PER_COMMENT ?? 15,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/frontend/apps/impress/src/features/docs/doc-editor/components/comments/useComments.ts`
at line 29, The code currently uses config?.REACTIONS_MAX_PER_COMMENT ?? 0 which
sets max reactions to 0 while config is still loading and prevents adding new
distinct emoji; change the fallback to the backend default (15) or gate
rendering until config is defined: update the expression to
config?.REACTIONS_MAX_PER_COMMENT ?? 15 (or wrap the reactions UI in a check
like if (config === undefined) show loading) and ensure callers such as
canAddReaction(comment, maxPerComment) receive the non-zero default or a
not-ready state.

),
);
}, [docId, canComment, provider?.awareness, user?.full_name]);
}, [
docId,
canComment,
provider?.awareness,
user?.full_name,
config?.REACTIONS_MAX_PER_COMMENT,
]);

useEffect(() => {
return () => {
Expand Down