🔒(backend) prevent admins from rewriting other users comments#2323
Merged
lunika merged 1 commit intoMay 26, 2026
Merged
Conversation
WalkthroughThis PR enforces comment authorship and separates editing vs deletion rights: serializers no longer inject Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 18: Move the entry "- 🐛(backend) prevent admins/owners from overwriting
other users' comments and silently claiming authorship" out of the general fixes
list and place it under a "Security" section (create a "Security" header if one
doesn't exist) so it appears alongside other security fixes (e.g., entries for
v5.1.0 and v4.0.0); keep the exact entry text but ensure it's grouped under the
Security heading and that changelog ordering/formatting matches the existing
release structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7084969a-2386-45f9-95ef-bc7da8de3631
📒 Files selected for processing (7)
CHANGELOG.mdsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets.pysrc/backend/core/models.pysrc/backend/core/tests/documents/test_api_documents_comments.pysrc/backend/core/tests/documents/test_api_documents_threads.pysrc/backend/core/tests/test_models_comment.py
💤 Files with no reviewable changes (1)
- src/backend/core/api/serializers.py
| ### Fixed | ||
|
|
||
| - 🐛(docs) run migration 0027 without superuser role | ||
| - 🐛(backend) prevent admins/owners from overwriting other users' comments and silently claiming authorship |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move this entry to a "Security" section.
This fix prevents admins/owners from impersonating other users by editing their comments and claiming authorship, which is a privilege escalation / authorization issue. Security fixes should be documented in a separate "Security" section (as seen in v5.1.0, v4.0.0, and other releases) for better visibility during security audits.
📝 Suggested changelog structure
### Fixed
- 🐛(docs) run migration 0027 without superuser role
-- 🐛(backend) prevent admins/owners from overwriting other users' comments and silently claiming authorship
-
+
+### Security
+
+- 🔒(backend) prevent admins/owners from overwriting other users' comments and silently claiming authorship
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 18, Move the entry "- 🐛(backend) prevent admins/owners
from overwriting other users' comments and silently claiming authorship" out of
the general fixes list and place it under a "Security" section (create a
"Security" header if one doesn't exist) so it appears alongside other security
fixes (e.g., entries for v5.1.0 and v4.0.0); keep the exact entry text but
ensure it's grouped under the Security heading and that changelog
ordering/formatting matches the existing release structure.
lunika
force-pushed
the
fix/comment-admin-impersonation
branch
2 times, most recently
from
1ddc805 to
310c7c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 18: Update the changelog entry string "- 🐛(backend) prevent
admins/owners from overwriting other users comments" to use the possessive
apostrophe: change "users comments" to "users' comments" so the line reads "-
🐛(backend) prevent admins/owners from overwriting other users' comments".
In `@src/backend/core/tests/test_models_comment.py`:
- Around line 268-280: The test
test_comment_get_abilities_user_admin_or_owner_other_comment is nondeterministic
because it uses random.choice(...) to create either an authored-by-other user or
an anonymous comment; replace that randomness by parametrizing the test over the
two author variants (e.g., author_user and None) so both branches run
deterministically for RoleChoices.ADMIN and RoleChoices.OWNER, creating comments
via factories.CommentFactory(thread__document=document, user=author) and
asserting the same abilities; after changing the parametrize, remove the
now-unused import that was only used for randomness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6f93681-c446-41c9-acc0-faa81bb62d79
📒 Files selected for processing (7)
CHANGELOG.mdsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets.pysrc/backend/core/models.pysrc/backend/core/tests/documents/test_api_documents_comments.pysrc/backend/core/tests/documents/test_api_documents_threads.pysrc/backend/core/tests/test_models_comment.py
💤 Files with no reviewable changes (1)
- src/backend/core/api/serializers.py
| ### Fixed | ||
|
|
||
| - 🐛(docs) run migration 0027 without superuser role | ||
| - 🐛(backend) prevent admins/owners from overwriting other users comments |
There was a problem hiding this comment.
Fix the grammatical error in the changelog entry.
The phrase "users comments" is missing the possessive apostrophe. It should be "users' comments" to correctly indicate that the comments belong to the users.
📝 Proposed fix
-- 🐛(backend) prevent admins/owners from overwriting other users comments
+- 🐛(backend) prevent admins/owners from overwriting other users' comments📝 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
| - 🐛(backend) prevent admins/owners from overwriting other users comments | |
| - 🐛(backend) prevent admins/owners from overwriting other users' comments |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 18, Update the changelog entry string "- 🐛(backend)
prevent admins/owners from overwriting other users comments" to use the
possessive apostrophe: change "users comments" to "users' comments" so the line
reads "- 🐛(backend) prevent admins/owners from overwriting other users'
comments".
Comment on lines
+268
to
+280
| @pytest.mark.parametrize("role", [RoleChoices.ADMIN, RoleChoices.OWNER]) | ||
| def test_comment_get_abilities_user_admin_or_owner_other_comment(role): | ||
| """Admins/owners can moderate others' comments (destroy) but cannot edit them.""" | ||
| user = factories.UserFactory() | ||
| document = factories.DocumentFactory(users=[(user, RoleChoices.OWNER)]) | ||
| document = factories.DocumentFactory(users=[(user, role)]) | ||
| comment = factories.CommentFactory( | ||
| thread__document=document, user=random.choice([user, None]) | ||
| thread__document=document, user=random.choice([factories.UserFactory(), None]) | ||
| ) | ||
|
|
||
| assert comment.get_abilities(user) == { | ||
| "destroy": True, | ||
| "update": True, | ||
| "partial_update": True, | ||
| "update": False, | ||
| "partial_update": False, |
There was a problem hiding this comment.
Make this test deterministic and cover both author variants explicitly.
At Line 274, random.choice(...) makes coverage nondeterministic: each run validates only one branch (other user or anonymous). Parametrize that axis so both cases are always exercised.
Proposed change
-@pytest.mark.parametrize("role", [RoleChoices.ADMIN, RoleChoices.OWNER])
-def test_comment_get_abilities_user_admin_or_owner_other_comment(role):
+@pytest.mark.parametrize("role", [RoleChoices.ADMIN, RoleChoices.OWNER])
+@pytest.mark.parametrize("other_author_is_anonymous", [False, True])
+def test_comment_get_abilities_user_admin_or_owner_other_comment(
+ role, other_author_is_anonymous
+):
"""Admins/owners can moderate others' comments (destroy) but cannot edit them."""
user = factories.UserFactory()
document = factories.DocumentFactory(users=[(user, role)])
- comment = factories.CommentFactory(
- thread__document=document, user=random.choice([factories.UserFactory(), None])
- )
+ other_author = None if other_author_is_anonymous else factories.UserFactory()
+ comment = factories.CommentFactory(thread__document=document, user=other_author)Also remove the now-unused import:
-import random📝 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
| @pytest.mark.parametrize("role", [RoleChoices.ADMIN, RoleChoices.OWNER]) | |
| def test_comment_get_abilities_user_admin_or_owner_other_comment(role): | |
| """Admins/owners can moderate others' comments (destroy) but cannot edit them.""" | |
| user = factories.UserFactory() | |
| document = factories.DocumentFactory(users=[(user, RoleChoices.OWNER)]) | |
| document = factories.DocumentFactory(users=[(user, role)]) | |
| comment = factories.CommentFactory( | |
| thread__document=document, user=random.choice([user, None]) | |
| thread__document=document, user=random.choice([factories.UserFactory(), None]) | |
| ) | |
| assert comment.get_abilities(user) == { | |
| "destroy": True, | |
| "update": True, | |
| "partial_update": True, | |
| "update": False, | |
| "partial_update": False, | |
| `@pytest.mark.parametrize`("role", [RoleChoices.ADMIN, RoleChoices.OWNER]) | |
| `@pytest.mark.parametrize`("other_author_is_anonymous", [False, True]) | |
| def test_comment_get_abilities_user_admin_or_owner_other_comment( | |
| role, other_author_is_anonymous | |
| ): | |
| """Admins/owners can moderate others' comments (destroy) but cannot edit them.""" | |
| user = factories.UserFactory() | |
| document = factories.DocumentFactory(users=[(user, role)]) | |
| other_author = None if other_author_is_anonymous else factories.UserFactory() | |
| comment = factories.CommentFactory(thread__document=document, user=other_author) | |
| assert comment.get_abilities(user) == { | |
| "destroy": True, | |
| "update": False, | |
| "partial_update": False, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/tests/test_models_comment.py` around lines 268 - 280, The
test test_comment_get_abilities_user_admin_or_owner_other_comment is
nondeterministic because it uses random.choice(...) to create either an
authored-by-other user or an anonymous comment; replace that randomness by
parametrizing the test over the two author variants (e.g., author_user and None)
so both branches run deterministically for RoleChoices.ADMIN and
RoleChoices.OWNER, creating comments via
factories.CommentFactory(thread__document=document, user=author) and asserting
the same abilities; after changing the parametrize, remove the now-unused import
that was only used for randomness.
Admins/owners could overwrite another user's comment text and silently take ownership of it. Split write permissions: destroy stays broad (author + admin/owner) for moderation. Signed-off-by: Mohamed El Amine BOUKERFA <boukerfa.ma@gmail.com>
lunika
force-pushed
the
fix/comment-admin-impersonation
branch
from
310c7c5 to
d8742c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/api/viewsets.py`:
- Around line 3007-3010: In perform_create, before calling
serializer.save(user=user), validate that the nested thread_id belongs to the
current resource by querying Thread with both id=thread_id and
document_id=resource_id (e.g. Thread.objects.filter(id=thread_id,
document_id=resource_id).exists() or get() inside a try/except); if no match,
raise a 404/400 (NotFound/ValidationError) so requests like POST
/documents/A/threads/<thread-from-B>/comments/ are rejected; keep the rest of
perform_create and only call serializer.save(user=user) after this check.
In `@src/backend/core/models.py`:
- Around line 1898-1906: The returned mutation flags
("destroy","update","partial_update") must require current document/read access
in addition to authorship or moderation; modify the logic in the method that
computes is_author and can_moderate so it also checks the user's current access
to the parent document (e.g. compute has_document_access via the existing
document permission helper or a method like document.can_read(user) or
DocumentPermission.has_object_permission) and only return true for
"update"/"partial_update" when is_author AND has_document_access, and only
return true for "destroy" when can_moderate AND has_document_access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 548e7202-10d2-480c-a3d4-f337b62c3848
📒 Files selected for processing (7)
CHANGELOG.mdsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets.pysrc/backend/core/models.pysrc/backend/core/tests/documents/test_api_documents_comments.pysrc/backend/core/tests/documents/test_api_documents_threads.pysrc/backend/core/tests/test_models_comment.py
Comment on lines
+3007
to
+3010
| def perform_create(self, serializer): | ||
| """Attach the request user as the comment author.""" | ||
| user = self.request.user if self.request.user.is_authenticated else None | ||
| serializer.save(user=user) |
There was a problem hiding this comment.
Reject cross-document thread IDs before saving the comment.
Line 3010 trusts the nested thread_id without proving that it belongs to resource_id. Since CommentPermission.has_permission() authorizes create from the document only, a user who can comment on document A can still POST to /documents/A/threads/<thread-from-B>/comments/ and write into document B if they know that thread UUID. Validate Thread(id=thread_id, document_id=resource_id) here and fail with 404/400 before calling save().
Suggested fix
def perform_create(self, serializer):
"""Attach the request user as the comment author."""
+ if not models.Thread.objects.filter(
+ id=self.kwargs["thread_id"],
+ document_id=self.kwargs["resource_id"],
+ ).exists():
+ raise drf.exceptions.NotFound("Thread not found.")
+
user = self.request.user if self.request.user.is_authenticated else None
serializer.save(user=user)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/api/viewsets.py` around lines 3007 - 3010, In
perform_create, before calling serializer.save(user=user), validate that the
nested thread_id belongs to the current resource by querying Thread with both
id=thread_id and document_id=resource_id (e.g.
Thread.objects.filter(id=thread_id, document_id=resource_id).exists() or get()
inside a try/except); if no match, raise a 404/400 (NotFound/ValidationError) so
requests like POST /documents/A/threads/<thread-from-B>/comments/ are rejected;
keep the rest of perform_create and only call serializer.save(user=user) after
this check.
Comment on lines
+1898
to
+1906
| is_author = self.user == user | ||
| can_moderate = is_author or role in [ | ||
| RoleChoices.OWNER, | ||
| RoleChoices.ADMIN, | ||
| ] | ||
| return { | ||
| "destroy": write_access, | ||
| "update": write_access, | ||
| "partial_update": write_access, | ||
| "destroy": can_moderate, | ||
| "update": is_author, | ||
| "partial_update": is_author, |
There was a problem hiding this comment.
Gate comment mutations on current document access as well as authorship.
Because CommentPermission.has_object_permission() delegates to this method, Lines 1904-1906 still allow an author to update or delete a comment after their access to the document has been revoked. That leaves a direct PATCH/DELETE path open for ex-members who know the nested URL. Please require current comment/read access in addition to authorship/moderation before returning mutation abilities.
Suggested fix
- is_author = self.user == user
- can_moderate = is_author or role in [
- RoleChoices.OWNER,
- RoleChoices.ADMIN,
- ]
+ is_author = read_access and self.user == user
+ can_moderate = read_access and (
+ self.user == user
+ or role in [
+ RoleChoices.OWNER,
+ RoleChoices.ADMIN,
+ ]
+ )
return {
"destroy": can_moderate,
"update": is_author,
"partial_update": is_author,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/models.py` around lines 1898 - 1906, The returned mutation
flags ("destroy","update","partial_update") must require current document/read
access in addition to authorship or moderation; modify the logic in the method
that computes is_author and can_moderate so it also checks the user's current
access to the parent document (e.g. compute has_document_access via the existing
document permission helper or a method like document.can_read(user) or
DocumentPermission.has_object_permission) and only return true for
"update"/"partial_update" when is_author AND has_document_access, and only
return true for "destroy" when can_moderate AND has_document_access.
lunika
approved these changes
May 26, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Currently, an administrator/owner can take ownership over other users comments by editing them:
when the comment is updated, the author field gets reassigned to the editor, silently rewriting authorship.
It makes sense for an admin to moderate comments by deleting them, but not to take ownership of them.
Screen.Recording.2026-05-25.at.21.56.40.mov