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
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ and this project adheres to
## [Unreleased]

### Added

- ✨(backend) add limit on distinct reactions per comment #1978
- ✨(backend) create a dedicated endpoint to update document content
- ⚡️(backend) stream s3 file content with a dedicated endpoint

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 @@ -2838,6 +2838,7 @@ def get(self, request):
"POSTHOG_KEY",
"LANGUAGES",
"LANGUAGE_CODE",
"REACTIONS_MAX_PER_COMMENT",
"SENTRY_DSN",
"TRASHBIN_CUTOFF_DAYS",
]
Expand Down Expand Up @@ -2955,7 +2956,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 @@ -2989,9 +2994,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 2996 to 3020
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 @@ -236,6 +236,11 @@ class Meta:
comment = factory.SubFactory(CommentFactory)
emoji = factory.Faker("emoji")

@classmethod
def generate_emojis(cls, n=10):
"""Generate a list of n unique emojis."""
return [fake.unique.emoji() for _ in range(n)]
Comment on lines +239 to +242
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Avoid shared fake.unique state across test cases.

Line 242 relies on a module-level unique registry, which can leak state between tests and make failures order-dependent.

♻️ Suggested refactor
 `@classmethod`
 def generate_emojis(cls, n=10):
     """Generate a list of n unique emojis."""
-    return [fake.unique.emoji() for _ in range(n)]
+    faker = Faker()
+    return [faker.unique.emoji() for _ in range(n)]
📝 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
@classmethod
def generate_emojis(cls, n=10):
"""Generate a list of n unique emojis."""
return [fake.unique.emoji() for _ in range(n)]
`@classmethod`
def generate_emojis(cls, n=10):
"""Generate a list of n unique emojis."""
faker = Faker()
return [faker.unique.emoji() for _ in range(n)]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/factories.py` around lines 239 - 242, The method
generate_emojis currently uses the module-level fake.unique registry which can
leak state between tests; change it to create and use a fresh Faker instance
inside the generate_emojis classmethod (e.g., instantiate Faker() locally and
call its .unique.emoji() to produce n emojis) so uniqueness is scoped to that
instance and does not share global state—replace uses of the module-level
fake.unique in generate_emojis with a local Faker().unique to isolate/avoid
cross-test leakage.


@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 @@ -934,3 +934,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 thread
coderabbitai[bot] marked this conversation as resolved.
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
1 change: 1 addition & 0 deletions src/backend/core/tests/test_api_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ def test_api_config(is_authenticated):
"LANGUAGE_CODE": "en-us",
"MEDIA_BASE_URL": "http://testserver/",
"POSTHOG_KEY": {"id": "132456", "host": "https://eu.i.posthog-test.com"},
"REACTIONS_MAX_PER_COMMENT": 15,
"SENTRY_DSN": "https://sentry.test/123",
"TRASHBIN_CUTOFF_DAYS": 30,
"theme_customization": {},
Expand Down
6 changes: 6 additions & 0 deletions src/backend/impress/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,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 @@ -56,6 +56,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 @@ -5,6 +5,7 @@ import { useCunninghamTheme } from '@/cunningham';
import { User, avatarUrlFromName } from '@/features/auth';
import { useEditorStore } from '@/features/docs/doc-editor/stores';
import { Doc, useProviderStore } from '@/features/docs/doc-management';
import { useConfig } from '@/core';
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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

const threadStore = useMemo(() => {
return new DocsThreadStore(
Expand All @@ -26,9 +28,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(() => {
if (canComment) {
Expand Down