fix(security): prevent identity leakage from anonymous doubts and replies#669
Open
prince-pokharna wants to merge 2 commits into
Open
fix(security): prevent identity leakage from anonymous doubts and replies#669prince-pokharna wants to merge 2 commits into
prince-pokharna wants to merge 2 commits into
Conversation
…anonymous doubt/reply API responses
|
@prince-pokharna is attempting to deploy a commit to the Karan Mani Tripathi 's projects Team on Vercel. A member of the Team first needs to authorize it. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
github-actions
Bot
added
gssoc'26
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughIntroduces ChangesAnonymous Privacy Sanitization
Homepage Scroll Progress Tracking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⭐ Star Required
Hi @prince-pokharna! Thank you for your contribution to DoubtDesk.
Before we can complete the review and merge this pull request, please star the DoubtDesk repository.
Once you have starred the repository, please drop a comment here saying "done" and we will proceed with reviewing your PR. Thank you!
|
@coderabbitai review |
There was a problem hiding this comment.
Hello there! 🎉 Thank you so much for your first pull request to DoubtDesk!
We really appreciate your contribution. A maintainer will review your code soon. If you are participating in GSSoC, ensure your PR is linked to an open issue. Please make sure you have followed all rules in our Contributing Guidelines. Happy coding!
codeant-ai
Bot
added
the
size:XXL
github-actions
Bot
added
size/xxl
and removed
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/page.tsx (1)
286-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Features anchor target mismatch with Header navigation.
src/components/Header.tsxlinks to#features-grid, but this section now exposesid="features"(Line 286). That breaks in-page “Features” navigation on home. Restorefeatures-gridhere or update Header links to#featuresconsistently.🤖 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/app/page.tsx` around lines 286 - 287, The section in page.tsx has id="features" but the Header.tsx component links to `#features-grid`, causing the anchor navigation to fail. Change the id attribute on this section from "features" to "features-grid" to match the navigation links in Header.tsx, ensuring the in-page Features navigation works correctly.src/lib/validations/doubt.ts (1)
22-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not require client-supplied
userNamefor likes.A like action should be tied to the authenticated server-side user, not a spoofable request-body name. This also pushes identity data back through the API surface during a privacy hardening PR.
🛡️ Suggested simplification
subject: trimmedString.max(100).optional(), imageUrl: z.union([safeUrl, z.literal('')]).optional().nullable().transform(e => e === '' ? null : e), - userName: trimmedString.optional().nullable(), // Add userName field replyId: positiveInt.optional().nullable(), tags: z.array(trimmedString.min(1).max(80)).max(8).optional(), status: z.enum(["unsolved", "in-progress", "solved"]).optional() -}).refine((data) => { - if (data.action === "like" && !data.userName) return false; - return true; -}, { - message: "User name required for like", - path: ["userName"] });🤖 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/lib/validations/doubt.ts` around lines 22 - 32, Remove the refine validation block that enforces userName requirement for like actions. The refine method currently validates that userName is provided when action is "like", but user identity for likes should come from server-side authentication context, not from client-supplied request body data. Delete the entire refine call (the section checking if data.action === "like" && !data.userName) to eliminate this client-side userName requirement.
🧹 Nitpick comments (2)
__tests__/anonymous-leak.test.ts (2)
8-8: ⚡ Quick winCover the exported
sanitizeRepliesbulk helper too.The array helper is used for list-style reply responses, but this suite only covers
sanitizeReplyandsanitizeDoubts. Add a mixedsanitizeRepliescase so regressions in bulk reply sanitization fail in the same privacy contract tests.🧪 Suggested test addition
-import { sanitizeDoubt, sanitizeReply, sanitizeDoubts } from "`@/lib/sanitize-response`"; +import { sanitizeDoubt, sanitizeReply, sanitizeDoubts, sanitizeReplies } from "`@/lib/sanitize-response`"; @@ describe("sanitizeDoubts — array of mixed doubts", () => { @@ }); + +describe("sanitizeReplies — array of mixed replies", () => { + const results = sanitizeReplies([anonymousReply], OTHER_USER_ID); + + test("sanitizes all replies in the array", () => { + expect(results).toHaveLength(1); + }); + + test("no reply contains author identity fields", () => { + for (const reply of results) { + expectNoAuthorIdentityFields(reply); + } + }); +});Also applies to: 201-227
🤖 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 `@__tests__/anonymous-leak.test.ts` at line 8, The test suite is missing coverage for the sanitizeReplies bulk helper which is used for list-style reply responses. Add sanitizeReplies to the import statement alongside sanitizeDoubt, sanitizeReply, and sanitizeDoubts, then add test cases for sanitizeReplies in the test suite to ensure regressions in bulk reply sanitization are caught by the privacy contract tests, similar to the existing tests for sanitizeReply and sanitizeDoubts.
97-227: ⚡ Quick winAssert forbidden identity keys, not only fixture values.
Because this file is the documented privacy test pattern, add a shared assertion for forbidden keys like
authorId,authorEmail,userEmail,userName,authorName,createdBy,clerkId, anduserId. The current string checks catch these fixtures, but can miss future leaks with different values or redacted-looking identifiers.🧪 Suggested helper
+const FORBIDDEN_AUTHOR_KEYS = [ + "authorId", + "authorEmail", + "authorName", + "userEmail", + "userName", + "createdBy", + "clerkId", + "userId", + "email", +]; + +function expectNoAuthorIdentityFields(value: unknown) { + const json = JSON.stringify(value); + for (const key of FORBIDDEN_AUTHOR_KEYS) { + expect(Object.keys(value as Record<string, unknown>)).not.toContain(key); + expect(json).not.toContain(`"${key}"`); + } +} + describe("sanitizeDoubt — anonymous post, different viewer", () => { const result = sanitizeDoubt(anonymousDoubt, OTHER_USER_ID); + test("does not contain forbidden author identity fields", () => { + expectNoAuthorIdentityFields(result); + }); +🤖 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 `@__tests__/anonymous-leak.test.ts` around lines 97 - 227, The current tests check only for specific fixture values (like AUTHOR_ID, email addresses, and names) but miss potential future leaks with different identity values. Create a shared helper function that asserts forbidden identity keys (authorId, authorEmail, userEmail, userName, authorName, createdBy, clerkId, userId) do not exist in the result objects. Replace or supplement the JSON.stringify checks in the describe blocks sanitizeDoubt, sanitizeReply, and sanitizeDoubts with this helper to comprehensively validate that no identity fields are present in the sanitized objects, regardless of their values.
🤖 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/__tests__/api/replies-auto-transition.test.ts`:
- Around line 211-274: The test assertion for updateSpy is incorrect because the
POST handler still calls db.update() for non-AI doubts, but protects solved
doubts through the WHERE clause filtering for isSolved = UNSOLVED, not by
skipping the update call entirely. Change the expectation from
expect(updateSpy).not.toHaveBeenCalled() to expect(updateSpy).toHaveBeenCalled()
since the update method is invoked, and then add an additional assertion to
verify that updateSpy was called with the correct WHERE conditions that filter
for isSolved = UNSOLVED, which prevents the update from affecting solved doubts.
In `@src/app/api/doubts/`[id]/route.ts:
- Around line 102-114: The displayName assignment on line 113 incorrectly uses
the current viewer's fullName (user?.fullName) instead of the doubt author's
name. Additionally, the isAnonymous logic on line 103 infers anonymity from
userEmail === null, which conflicts with the privacy model; instead, use the
existing anonymous flag already stored in the doubt object. Fix this by removing
the inferred isAnonymous calculation and instead directly use doubt.isAnonymous
(or the appropriate field name from the doubt object), and replace
user?.fullName with the doubt author's actual name field from the doubt object
(such as doubt.authorName or doubt.userName) to ensure the displayName reflects
the post author's identity, not the current requester's.
In `@src/app/api/doubts/route.ts`:
- Line 42: The sanitizeDoubt and sanitizeDoubts functions compare ownership
against doubt.userEmail, but userId is currently being set to user?.id (Clerk's
user ID), causing isOwnPost to incorrectly return false for the actual author.
Update all four occurrences where userId is assigned (on lines 42, 252, 277, and
424) to use the user's email address instead of user.id, so the ownership
comparison in the sanitizer functions will correctly match against the
doubt.userEmail field.
In `@src/app/api/replies/route.ts`:
- Around line 36-37: The replies route is passing user.id to the sanitizeReply
and sanitizeReplies functions, but these functions determine ownership by
comparing against reply.userEmail. This mismatch breaks ownership detection in
sanitized responses. Replace the userId assignment to pass user.email instead of
user.id so that the ownership check in sanitizeReply and sanitizeReplies can
correctly identify the post owner by matching email values. Apply this fix at
all locations mentioned (lines 36-37, 104-106, 131-132, and 305-307) where
userId is assigned.
In `@src/app/page.tsx`:
- Line 45: Remove the duplicate scroll-to-top implementation from the Home page
to avoid duplicating the control already provided by RootLayout's
ScrollToTopButton component. Delete the scrollProgress state variable
declaration, remove the scroll event listener logic that updates scroll progress
(lines 62-73), and delete the scroll-to-top button rendering and related styling
code (lines 420-456). The shared ScrollToTopButton from RootLayout will handle
all scroll-to-top functionality for this page.
In `@src/lib/sanitize-response.ts`:
- Around line 40-66: Remove the raw author identity fields from the safeFields
whitelist array in the sanitize-response.ts file. Specifically, delete
`authorName`, `createdBy`, and `userName` from the safeFields array (lines
40-66), as these fields expose real author identity and should never be directly
exposed in API responses. These fields should only be used as private inputs for
deriving the non-anonymous `displayName` field, not as raw response fields. Also
apply the same removal to any other similar whitelist arrays in the file (also
check the section around lines 149-160 mentioned in the comment).
- Around line 30-32: The isOwnPost comparison is using mismatched identifiers:
currentUserId (Clerk user.id) is being compared against doubt.userEmail instead
of the stored author identifier that matches the caller's identifier type.
Update the comparison in the isOwnPost assignment to compare currentUserId
against a stored Clerk user ID or author ID field in the doubt object, not
userEmail. Apply this same fix to all locations where isOwnPost is computed
(including the location at lines 143-144 mentioned in the comment) to ensure
consistent ownership checks across all helpers.
---
Outside diff comments:
In `@src/app/page.tsx`:
- Around line 286-287: The section in page.tsx has id="features" but the
Header.tsx component links to `#features-grid`, causing the anchor navigation to
fail. Change the id attribute on this section from "features" to "features-grid"
to match the navigation links in Header.tsx, ensuring the in-page Features
navigation works correctly.
In `@src/lib/validations/doubt.ts`:
- Around line 22-32: Remove the refine validation block that enforces userName
requirement for like actions. The refine method currently validates that
userName is provided when action is "like", but user identity for likes should
come from server-side authentication context, not from client-supplied request
body data. Delete the entire refine call (the section checking if data.action
=== "like" && !data.userName) to eliminate this client-side userName
requirement.
---
Nitpick comments:
In `@__tests__/anonymous-leak.test.ts`:
- Line 8: The test suite is missing coverage for the sanitizeReplies bulk helper
which is used for list-style reply responses. Add sanitizeReplies to the import
statement alongside sanitizeDoubt, sanitizeReply, and sanitizeDoubts, then add
test cases for sanitizeReplies in the test suite to ensure regressions in bulk
reply sanitization are caught by the privacy contract tests, similar to the
existing tests for sanitizeReply and sanitizeDoubts.
- Around line 97-227: The current tests check only for specific fixture values
(like AUTHOR_ID, email addresses, and names) but miss potential future leaks
with different identity values. Create a shared helper function that asserts
forbidden identity keys (authorId, authorEmail, userEmail, userName, authorName,
createdBy, clerkId, userId) do not exist in the result objects. Replace or
supplement the JSON.stringify checks in the describe blocks sanitizeDoubt,
sanitizeReply, and sanitizeDoubts with this helper to comprehensively validate
that no identity fields are present in the sanitized objects, regardless of
their values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19f70f14-2790-4bf2-b696-b243ad08c688
📒 Files selected for processing (11)
README.md__tests__/anonymous-leak.test.tssrc/__tests__/api/replies-auto-transition.test.tssrc/app/api/doubts/[id]/route.tssrc/app/api/doubts/route.tssrc/app/api/replies/route.tssrc/app/api/rooms/[id]/route.tssrc/app/page.tsxsrc/lib/sanitize-response.tssrc/lib/validations/doubt.tssrc/types/api.ts
Comment on lines
+211
to
+274
| it("solved doubt is NEVER downgraded by a new reply", async () => { | ||
| const doubtId = 3; | ||
|
|
||
| const mockDb = require("@/configs/db").db; | ||
|
|
||
| // Mock user block check | ||
| mockDb.select.mockReturnValueOnce({ | ||
| from: jest.fn().mockReturnValue({ | ||
| where: jest.fn().mockResolvedValue([]), | ||
| }), | ||
| }); | ||
|
|
||
| // Mock doubt check - solved doubt | ||
| mockDb.select.mockReturnValueOnce({ | ||
| from: jest.fn().mockReturnValue({ | ||
| where: jest.fn().mockResolvedValue([{ | ||
| id: doubtId, | ||
| userEmail: "author@example.com", | ||
| classroomId: null, | ||
| type: "community", | ||
| isSolved: DOUBT_STATUS.SOLVED, | ||
| subject: "Test Subject", | ||
| content: "Test Content", | ||
| }]), | ||
| }), | ||
| }); | ||
|
|
||
| // Mock reply insert | ||
| const insertSpy = jest.fn().mockResolvedValue([{ | ||
| id: 3, | ||
| doubtId, | ||
| userEmail: "test@example.com", | ||
| type: "community", | ||
| content: "Reply to solved", | ||
| createdAt: new Date() | ||
| }]); | ||
| mockDb.insert.mockReturnValue({ | ||
| values: jest.fn().mockReturnValue({ | ||
| returning: insertSpy, | ||
| }), | ||
| }); | ||
|
|
||
| // Mock doubt update - should not be called for solved doubts | ||
| const updateSpy = jest.fn(); | ||
| mockDb.update.mockReturnValue({ | ||
| set: jest.fn().mockReturnValue({ | ||
| where: updateSpy, | ||
| }), | ||
| }); | ||
|
|
||
| const req = mockReq({ doubtId, type: "community", content: "Reply to solved" }); | ||
| const response = await POST(req); | ||
| const json = await response.json(); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(json).toHaveProperty("id"); | ||
| expect(insertSpy).toHaveBeenCalled(); | ||
| // The update should NOT be called because the doubt is solved | ||
| // and the code checks if (doubt && doubt.type !== "ai") before attempting update | ||
| // But since doubt.isSolved === DOUBT_STATUS.SOLVED, the WHERE clause won't match | ||
| // The updateSpy might be called but won't update anything | ||
| // Let's check that it's called with the correct WHERE conditions | ||
| expect(updateSpy).not.toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Solved-case assertion conflicts with current POST flow.
For non-AI doubts, the handler still invokes db.update(...where(isSolved = UNSOLVED)); solved doubts are protected by the WHERE clause, not by skipping the call entirely. This assertion currently expects the opposite call pattern.
💡 Suggested patch
- expect(updateSpy).not.toHaveBeenCalled();
+ expect(updateSpy).toHaveBeenCalled();📝 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
| it("solved doubt is NEVER downgraded by a new reply", async () => { | |
| const doubtId = 3; | |
| const mockDb = require("@/configs/db").db; | |
| // Mock user block check | |
| mockDb.select.mockReturnValueOnce({ | |
| from: jest.fn().mockReturnValue({ | |
| where: jest.fn().mockResolvedValue([]), | |
| }), | |
| }); | |
| // Mock doubt check - solved doubt | |
| mockDb.select.mockReturnValueOnce({ | |
| from: jest.fn().mockReturnValue({ | |
| where: jest.fn().mockResolvedValue([{ | |
| id: doubtId, | |
| userEmail: "author@example.com", | |
| classroomId: null, | |
| type: "community", | |
| isSolved: DOUBT_STATUS.SOLVED, | |
| subject: "Test Subject", | |
| content: "Test Content", | |
| }]), | |
| }), | |
| }); | |
| // Mock reply insert | |
| const insertSpy = jest.fn().mockResolvedValue([{ | |
| id: 3, | |
| doubtId, | |
| userEmail: "test@example.com", | |
| type: "community", | |
| content: "Reply to solved", | |
| createdAt: new Date() | |
| }]); | |
| mockDb.insert.mockReturnValue({ | |
| values: jest.fn().mockReturnValue({ | |
| returning: insertSpy, | |
| }), | |
| }); | |
| // Mock doubt update - should not be called for solved doubts | |
| const updateSpy = jest.fn(); | |
| mockDb.update.mockReturnValue({ | |
| set: jest.fn().mockReturnValue({ | |
| where: updateSpy, | |
| }), | |
| }); | |
| const req = mockReq({ doubtId, type: "community", content: "Reply to solved" }); | |
| const response = await POST(req); | |
| const json = await response.json(); | |
| expect(response.status).toBe(200); | |
| expect(json).toHaveProperty("id"); | |
| expect(insertSpy).toHaveBeenCalled(); | |
| // The update should NOT be called because the doubt is solved | |
| // and the code checks if (doubt && doubt.type !== "ai") before attempting update | |
| // But since doubt.isSolved === DOUBT_STATUS.SOLVED, the WHERE clause won't match | |
| // The updateSpy might be called but won't update anything | |
| // Let's check that it's called with the correct WHERE conditions | |
| expect(updateSpy).not.toHaveBeenCalled(); | |
| }); | |
| it("solved doubt is NEVER downgraded by a new reply", async () => { | |
| const doubtId = 3; | |
| const mockDb = require("`@/configs/db`").db; | |
| // Mock user block check | |
| mockDb.select.mockReturnValueOnce({ | |
| from: jest.fn().mockReturnValue({ | |
| where: jest.fn().mockResolvedValue([]), | |
| }), | |
| }); | |
| // Mock doubt check - solved doubt | |
| mockDb.select.mockReturnValueOnce({ | |
| from: jest.fn().mockReturnValue({ | |
| where: jest.fn().mockResolvedValue([{ | |
| id: doubtId, | |
| userEmail: "author@example.com", | |
| classroomId: null, | |
| type: "community", | |
| isSolved: DOUBT_STATUS.SOLVED, | |
| subject: "Test Subject", | |
| content: "Test Content", | |
| }]), | |
| }), | |
| }); | |
| // Mock reply insert | |
| const insertSpy = jest.fn().mockResolvedValue([{ | |
| id: 3, | |
| doubtId, | |
| userEmail: "test@example.com", | |
| type: "community", | |
| content: "Reply to solved", | |
| createdAt: new Date() | |
| }]); | |
| mockDb.insert.mockReturnValue({ | |
| values: jest.fn().mockReturnValue({ | |
| returning: insertSpy, | |
| }), | |
| }); | |
| // Mock doubt update - should not be called for solved doubts | |
| const updateSpy = jest.fn(); | |
| mockDb.update.mockReturnValue({ | |
| set: jest.fn().mockReturnValue({ | |
| where: updateSpy, | |
| }), | |
| }); | |
| const req = mockReq({ doubtId, type: "community", content: "Reply to solved" }); | |
| const response = await POST(req); | |
| const json = await response.json(); | |
| expect(response.status).toBe(200); | |
| expect(json).toHaveProperty("id"); | |
| expect(insertSpy).toHaveBeenCalled(); | |
| // The update should NOT be called because the doubt is solved | |
| // and the code checks if (doubt && doubt.type !== "ai") before attempting update | |
| // But since doubt.isSolved === DOUBT_STATUS.SOLVED, the WHERE clause won't match | |
| // The updateSpy might be called but won't update anything | |
| // Let's check that it's called with the correct WHERE conditions | |
| expect(updateSpy).toHaveBeenCalled(); | |
| }); |
🤖 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/__tests__/api/replies-auto-transition.test.ts` around lines 211 - 274,
The test assertion for updateSpy is incorrect because the POST handler still
calls db.update() for non-AI doubts, but protects solved doubts through the
WHERE clause filtering for isSolved = UNSOLVED, not by skipping the update call
entirely. Change the expectation from expect(updateSpy).not.toHaveBeenCalled()
to expect(updateSpy).toHaveBeenCalled() since the update method is invoked, and
then add an additional assertion to verify that updateSpy was called with the
correct WHERE conditions that filter for isSolved = UNSOLVED, which prevents the
update from affecting solved doubts.
Comment on lines
+102
to
+114
| // Determine if the doubt is anonymous | ||
| const isAnonymous = doubt.userEmail === null || doubt.type === 'anonymous'; | ||
|
|
||
| // Prepare the doubt with all fields, ensuring defaults | ||
| const doubtWithDetails = { | ||
| ...doubt, | ||
| tags, | ||
| hasLiked, | ||
| hasBookmarked, | ||
| }); | ||
| tags: tags || [], | ||
| hasLiked: hasLiked || false, | ||
| hasBookmarked: hasBookmarked || false, | ||
| isAnonymous: isAnonymous, | ||
| // Add displayName for the sanitization function to use | ||
| displayName: isAnonymous ? 'Anonymous' : user?.fullName || 'User', | ||
| }; |
Contributor
There was a problem hiding this comment.
Derive anonymity and display name from the post author data, not the requester.
For non-anonymous doubts, Line 113 uses user?.fullName, which is the current viewer’s name, not the doubt author’s. Also, inferring anonymity from userEmail === null conflicts with the privacy model where author identity remains stored server-side for moderation; preserve the stored anonymous flag/handle instead of overwriting it here.
🐛 Suggested direction
- // Determine if the doubt is anonymous
- const isAnonymous = doubt.userEmail === null || doubt.type === 'anonymous';
+ // Determine anonymity from the persisted post metadata/handle, not requester state.
+ const isAnonymous = Boolean(doubt.isAnonymous);
@@
- isAnonymous: isAnonymous,
- // Add displayName for the sanitization function to use
- displayName: isAnonymous ? 'Anonymous' : user?.fullName || 'User',
+ isAnonymous,
+ // Populate this from the post's author/anonymous handle data selected above.
+ displayName: isAnonymous
+ ? doubt.displayName ?? doubt.anonymousHandle ?? 'Anonymous'
+ : doubt.displayName ?? doubt.userName ?? 'User',📝 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
| // Determine if the doubt is anonymous | |
| const isAnonymous = doubt.userEmail === null || doubt.type === 'anonymous'; | |
| // Prepare the doubt with all fields, ensuring defaults | |
| const doubtWithDetails = { | |
| ...doubt, | |
| tags, | |
| hasLiked, | |
| hasBookmarked, | |
| }); | |
| tags: tags || [], | |
| hasLiked: hasLiked || false, | |
| hasBookmarked: hasBookmarked || false, | |
| isAnonymous: isAnonymous, | |
| // Add displayName for the sanitization function to use | |
| displayName: isAnonymous ? 'Anonymous' : user?.fullName || 'User', | |
| }; | |
| // Determine anonymity from the persisted post metadata/handle, not requester state. | |
| const isAnonymous = Boolean(doubt.isAnonymous); | |
| // Prepare the doubt with all fields, ensuring defaults | |
| const doubtWithDetails = { | |
| ...doubt, | |
| tags: tags || [], | |
| hasLiked: hasLiked || false, | |
| hasBookmarked: hasBookmarked || false, | |
| isAnonymous, | |
| // Populate this from the post's author/anonymous handle data selected above. | |
| displayName: isAnonymous | |
| ? doubt.displayName ?? doubt.anonymousHandle ?? 'Anonymous' | |
| : doubt.displayName ?? doubt.userName ?? '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/app/api/doubts/`[id]/route.ts around lines 102 - 114, The displayName
assignment on line 113 incorrectly uses the current viewer's fullName
(user?.fullName) instead of the doubt author's name. Additionally, the
isAnonymous logic on line 103 infers anonymity from userEmail === null, which
conflicts with the privacy model; instead, use the existing anonymous flag
already stored in the doubt object. Fix this by removing the inferred
isAnonymous calculation and instead directly use doubt.isAnonymous (or the
appropriate field name from the doubt object), and replace user?.fullName with
the doubt author's actual name field from the doubt object (such as
doubt.authorName or doubt.userName) to ensure the displayName reflects the post
author's identity, not the current requester's.
| try { | ||
| const user = await currentUser(); | ||
| const email = user?.primaryEmailAddress?.emailAddress ?? null; | ||
| const userId = user?.id ?? null; |
Contributor
There was a problem hiding this comment.
Align sanitizer identity key with the field it compares.
sanitizeDoubt/sanitizeDoubts currently compute ownership by comparing against doubt.userEmail. Passing Clerk user.id here makes isOwnPost false for the actual author.
💡 Suggested patch
- const userId = user?.id ?? null;
+ const userIdentity = email ?? null;
...
- const safeDoubts = sanitizeDoubts(doubts, userId);
+ const safeDoubts = sanitizeDoubts(doubts, userIdentity);
...
- const userId = user?.id ?? null;
+ const userIdentity = email ?? null;
...
- const safeDoubt = sanitizeDoubt(doubtWithTags, userId);
+ const safeDoubt = sanitizeDoubt(doubtWithTags, userIdentity);Also applies to: 252-252, 277-277, 424-424
🤖 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/app/api/doubts/route.ts` at line 42, The sanitizeDoubt and sanitizeDoubts
functions compare ownership against doubt.userEmail, but userId is currently
being set to user?.id (Clerk's user ID), causing isOwnPost to incorrectly return
false for the actual author. Update all four occurrences where userId is
assigned (on lines 42, 252, 277, and 424) to use the user's email address
instead of user.id, so the ownership comparison in the sanitizer functions will
correctly match against the doubt.userEmail field.
Comment on lines
+36
to
37
| const userId = user?.id ?? null; | ||
|
|
Contributor
There was a problem hiding this comment.
Same ownership-key mismatch is present in replies sanitization.
The route passes Clerk user.id, but sanitizeReply/sanitizeReplies determine isOwnPost against reply.userEmail. That prevents true owner detection in sanitized reply responses.
💡 Suggested patch
- const userId = user?.id ?? null;
+ const userIdentity = email ?? null;
...
- const safeReplies = sanitizeReplies(repliesWithVotes, userId);
+ const safeReplies = sanitizeReplies(repliesWithVotes, userIdentity);
...
- const userId = user?.id ?? null;
+ const userIdentity = email ?? null;
...
- const safeReply = sanitizeReply(newReply[0], userId);
+ const safeReply = sanitizeReply(newReply[0], userIdentity);Also applies to: 104-106, 131-132, 305-307
🤖 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/app/api/replies/route.ts` around lines 36 - 37, The replies route is
passing user.id to the sanitizeReply and sanitizeReplies functions, but these
functions determine ownership by comparing against reply.userEmail. This
mismatch breaks ownership detection in sanitized responses. Replace the userId
assignment to pass user.email instead of user.id so that the ownership check in
sanitizeReply and sanitizeReplies can correctly identify the post owner by
matching email values. Apply this fix at all locations mentioned (lines 36-37,
104-106, 131-132, and 305-307) where userId is assigned.
|
|
||
| export default function Home() { | ||
| const [showSignOutDialog, setShowSignOutDialog] = useState(false); | ||
| const [scrollProgress, setScrollProgress] = useState(0); |
Contributor
There was a problem hiding this comment.
Avoid rendering a second scroll-to-top control on Home.
RootLayout already mounts ScrollToTopButton, and this page now adds another button with the same scroll-progress-driven visibility. On /, users can get duplicate floating controls and duplicated listeners/logic. Prefer reusing the shared component and remove this page-local implementation.
Also applies to: 62-73, 420-456
🤖 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/app/page.tsx` at line 45, Remove the duplicate scroll-to-top
implementation from the Home page to avoid duplicating the control already
provided by RootLayout's ScrollToTopButton component. Delete the scrollProgress
state variable declaration, remove the scroll event listener logic that updates
scroll progress (lines 62-73), and delete the scroll-to-top button rendering and
related styling code (lines 420-456). The shared ScrollToTopButton from
RootLayout will handle all scroll-to-top functionality for this page.
Comment on lines
+40
to
+66
| // Fields that are safe to expose - include all possible public fields | ||
| const safeFields = [ | ||
| 'id', | ||
| 'subject', | ||
| 'subTopic', | ||
| 'content', | ||
| 'imageUrl', | ||
| 'classroomId', | ||
| 'type', | ||
| 'isSolved', | ||
| 'isPinned', | ||
| 'likes', | ||
| 'likeCount', | ||
| 'replyCount', | ||
| 'createdAt', | ||
| 'updatedAt', | ||
| 'deletedAt', | ||
| 'tags', | ||
| 'hasLiked', | ||
| 'hasBookmarked', | ||
| 'isAnonymous', | ||
| 'isResolved', | ||
| 'displayName', | ||
| 'userName', | ||
| 'createdBy', | ||
| 'authorName' | ||
| ]; |
Contributor
There was a problem hiding this comment.
Remove raw author-name fields from the public whitelist.
authorName, createdBy, and userName can carry real author identity and are copied directly into the API response. For anonymous rows this reintroduces the exact leak the sanitizer is meant to prevent; use those fields only as private inputs for non-anonymous displayName derivation, never as raw response fields.
🛡️ Suggested whitelist tightening
'isResolved',
'displayName',
- 'userName',
- 'createdBy',
- 'authorName'
];
@@
'updatedAt',
'isAnonymous',
- 'displayName',
- 'userName'
+ 'displayName'
]; delete safeDoubt.author_email;
+ delete safeDoubt.authorEmail;
+ delete safeDoubt.authorName;
+ delete safeDoubt.createdBy;
delete safeDoubt.email;
+ delete safeDoubt.userName;
delete safeDoubt.user_id;
@@
delete safeReply.author_email;
+ delete safeReply.authorEmail;
+ delete safeReply.authorName;
delete safeReply.email;
+ delete safeReply.userName;
delete safeReply.user_id;Also applies to: 149-160
🤖 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/lib/sanitize-response.ts` around lines 40 - 66, Remove the raw author
identity fields from the safeFields whitelist array in the sanitize-response.ts
file. Specifically, delete `authorName`, `createdBy`, and `userName` from the
safeFields array (lines 40-66), as these fields expose real author identity and
should never be directly exposed in API responses. These fields should only be
used as private inputs for deriving the non-anonymous `displayName` field, not
as raw response fields. Also apply the same removal to any other similar
whitelist arrays in the file (also check the section around lines 149-160
mentioned in the comment).
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| // Determine if the current user is the author | ||
| // Use the userEmail field for comparison (stored in DB but not exposed) | ||
| const isOwnPost = currentUserId ? doubt.userEmail === currentUserId : false; |
There was a problem hiding this comment.
Suggestion: isOwnPost is computed by comparing currentUserId with userEmail, but those identifiers are different types (Clerk user id vs email). This makes ownership detection incorrect and will mark a user's own post/reply as not theirs. Compare against email consistently (or pass an author id field and compare id-to-id). [api mismatch]
Severity Level: Major ⚠️
- ⚠️ `isOwnPost` flag always false for real authors.
- ⚠️ Future clients cannot trust ownership information.
- ⚠️ Anonymity tests expecting true ownership semantics will fail.Steps of Reproduction ✅
1. In the doubts listing endpoint `src/app/api/doubts/route.ts:28-260`, a logged-in user
is resolved via Clerk at lines 39-43. The handler derives `email =
user?.primaryEmailAddress?.emailAddress ?? null` and separately `userId = user?.id ??
null`.
2. The same handler builds `doubts` from `doubtsTable` (schema in
`src/configs/schema.ts:181-220`) where the author is stored in the `userEmail` column
(string email), e.g. `"student@example.com"`.
3. Before returning, the handler calls `sanitizeDoubts(doubts, userId)` at
`src/app/api/doubts/route.ts:252`, passing the Clerk user id (e.g. `"user_clerk_abc123"`)
as `currentUserId` into `sanitizeDoubt()` in `src/lib/sanitize-response.ts:24-32`.
4. In `sanitizeDoubt`, `isOwnPost` is computed as `const isOwnPost = currentUserId ?
doubt.userEmail === currentUserId : false;` (line 32), effectively comparing
`"student@example.com" === "user_clerk_abc123"`. This is always `false` in production, so
every sanitized doubt in API responses (and similarly every reply via `sanitizeReplies()`
in `src/app/api/replies/route.ts:89-106`) has `isOwnPost` set to `false`, even when the
requesting user is the author. The unit tests in
`__tests__/anonymous-leak.test.ts:129-133` also assume that passing an author identifier
should produce `isOwnPost === true`, which this implementation does not satisfy.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/sanitize-response.ts
**Line:** 32:32
**Comment:**
*Api Mismatch: `isOwnPost` is computed by comparing `currentUserId` with `userEmail`, but those identifiers are different types (Clerk user id vs email). This makes ownership detection incorrect and will mark a user's own post/reply as not theirs. Compare against email consistently (or pass an author id field and compare id-to-id).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+64
to
+65
| 'createdBy', | ||
| 'authorName' |
There was a problem hiding this comment.
Suggestion: authorName is explicitly whitelisted as a safe field, so any caller that includes raw author name in the source object will leak identity even for anonymous posts. Remove authorName from allowed fields (or conditionally expose only for non-anonymous content after strict checks). [security]
Severity Level: Critical 🚨
- ❌ Anonymous test data leaks real `authorName` field.
- ⚠️ Future joined queries could expose real names for anonymous doubts.
- ⚠️ Violates documented privacy guarantees for anonymous content.Steps of Reproduction ✅
1. The test fixture `anonymousDoubt` in `__tests__/anonymous-leak.test.ts:47-62`
represents an anonymous post and includes real identity fields: `authorId`, `authorName:
"Rahul Sharma"`, and `authorEmail: "rahul@college.edu"`, plus `isAnonymous: true` and
`userEmail: null`.
2. The test `"sanitizeDoubt — anonymous post, different viewer"` at
`__tests__/anonymous-leak.test.ts:97-127` calls `sanitizeDoubt(anonymousDoubt,
OTHER_USER_ID)` where `OTHER_USER_ID` is a different Clerk user id.
3. In `sanitizeDoubt` (`src/lib/sanitize-response.ts:37-75`), `sensitiveFields` does not
include `authorName`, and `safeFields` at lines 41-66 explicitly includes `'authorName'`.
The copy loop `for (const field of safeFields)` therefore copies `doubt.authorName` into
`safeDoubt.authorName`.
4. The test then asserts at `__tests__/anonymous-leak.test.ts:104-106` that
`JSON.stringify(result)` should not contain `"Rahul Sharma"`. Because `authorName` is
copied straight through, the real name appears in the sanitized object and in any JSON API
responses built from that object, leaking the author's identity for anonymous content.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/sanitize-response.ts
**Line:** 64:65
**Comment:**
*Security: `authorName` is explicitly whitelisted as a safe field, so any caller that includes raw author name in the source object will leak identity even for anonymous posts. Remove `authorName` from allowed fields (or conditionally expose only for non-anonymous content after strict checks).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
|
||
| const safeDoubts = sanitizeDoubts(doubts, userId); | ||
|
|
||
| return NextResponse.json({ |
There was a problem hiding this comment.
Suggestion: This API now returns sanitized doubts that drop userEmail, but existing UI consumers still derive owner state and author label from doubt.userEmail, so the list UI will lose ownership detection and author rendering. Either migrate consumers to displayName/isOwnPost in the same change or keep a compatible non-sensitive field contract until clients are updated. [api mismatch]
Severity Level: Critical 🚨
- ❌ Owner-only actions hidden on doubt cards list.
- ⚠️ "You" badge never appears for authors in feeds.
- ⚠️ All doubts appear anonymous regardless of original visibility.
- ⚠️ React components silently mismatch updated API contract.Steps of Reproduction ✅
1. Trigger the doubts listing API `GET /api/doubts` implemented in
`src/app/api/doubts/route.ts:28-260`. The handler builds `doubts` from Drizzle rows and
then calls `const safeDoubts = sanitizeDoubts(doubts, userId);` before returning
`NextResponse.json({ doubts: safeDoubts, hasMore, totalCount, page, limit });` at lines
252-260.
2. Inside `sanitizeDoubts()` / `sanitizeDoubt()` in `src/lib/sanitize-response.ts:24-121`,
the `sensitiveFields` array includes `"userEmail"` and the copy loop only transfers
whitelisted, non-sensitive fields. As a result, each item in `safeDoubts` no longer has a
`userEmail` property, even though the original Drizzle rows did.
3. On the client, `src/components/InfiniteDoubtFeed.tsx:8-16, 60-99` fetches
`/api/doubts?...` via `fetcher()`, does `return res.json()`, and then flattens
`pages.flatMap(page => page.doubts)` where each `doubt` is typed as `Doubt` from
`src/types/index.ts:200-217` (which still includes `userEmail?: string | null`).
4. Each `doubt` is passed into `DoubtCard` (`src/components/DoubtCard.tsx:40-62`).
`DoubtCard` derives ownership and author label from `doubt.userEmail`: `useEffect` at
lines 80-86 compares `user?.primaryEmailAddress?.emailAddress === doubt.userEmail` to set
`isOwner`, and the header at lines 231-239 renders the avatar initial and handle from
`doubt.userEmail`. With sanitized API data, `doubt.userEmail` is always `undefined`, so
`isOwner` stays `false` (removing the "You" badge and owner-only actions like the "Mark
Solved" button at lines 395-403) and the header falls back to `'?'` and `'Anonymous'` for
all doubts, changing runtime behavior for all existing list and bookmark UIs consuming
`/api/doubts`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/doubts/route.ts
**Line:** 254:260
**Comment:**
*Api Mismatch: This API now returns sanitized doubts that drop `userEmail`, but existing UI consumers still derive owner state and author label from `doubt.userEmail`, so the list UI will lose ownership detection and author rendering. Either migrate consumers to `displayName`/`isOwnPost` in the same change or keep a compatible non-sensitive field contract until clients are updated.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Owner
|
@prince-pokharna resolve merge conflicts and also look into suggestions from coderabbit and codeant to improve the code quality |
github-actions
Bot
added
the
merge-conflict
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XXL
github-actions
Bot
removed
size:XXL
Author
|
Hey @knoxiboy , Thank you for assigning me the issue. |
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.
User description
Summary
Fixes a privacy vulnerability where anonymous doubt and reply API responses could expose author identity information through raw database fields returned by API endpoints.
Although anonymous content appeared correctly anonymized in the UI, sensitive fields such as author identifiers and author metadata could still be present in JSON responses returned by the backend. This allowed users to inspect network responses and potentially recover the identity behind anonymous content.
This PR introduces response sanitization to ensure sensitive author information is never exposed through API responses.
Root Cause
Anonymous posting was enforced only at the presentation layer.
Several API routes returned raw Drizzle query results directly via
NextResponse.json(). While the frontend displayed anonymous handles, the underlying API responses could still contain author-related fields.As a result, anonymity depended on the frontend implementation rather than being enforced by the backend.
What Changed
File | Change -- | -- lib/sanitize-response.ts | Added centralized sanitization helpers for doubts and replies app/api/doubts/route.ts | Apply response sanitization before returning doubts app/api/doubts/[id]/route.ts | Apply sanitization before returning individual doubts and enforce ownership checks server-side app/api/doubts/[id]/replies/route.ts | Apply reply sanitization before returning replies app/api/rooms/[roomId]/doubts/route.ts | Apply sanitization before returning classroom doubts __tests__/anonymous-leak.test.ts | Added tests covering anonymous content privacy scenarios README.md | Documented privacy guarantees and response sanitization requirements CONTRIBUTING.md | Added guidance for sanitizing user-facing API responsesPrivacy Model
The following rules are now enforced by the backend:
Author identity data remains stored in the database for moderation, ownership validation, and abuse prevention.
Sensitive author fields are never returned in public API responses.
Ownership is communicated using an
isOwnPostboolean flag instead of exposing internal identifiers.Anonymous content exposes only its anonymous display handle.
Public (non-anonymous) content retains existing behavior.
Security Impact
This change prevents:
Identity disclosure through browser DevTools
Exposure of internal author identifiers
Leakage of personally identifiable information from anonymous content
Client-side bypasses of anonymity protections
Privacy enforcement now occurs at the API layer rather than relying solely on frontend rendering.
Testing
All tests pass successfully.
Test coverage includes:
Anonymous doubt sanitization
Anonymous reply sanitization
Ownership detection (
isOwnPost)Public post behavior
Removal of sensitive author fields
API response sanitization paths
Manual Verification
Sign in and create an anonymous doubt.
Open browser DevTools → Network tab.
Inspect the response from
GET /api/doubts.Verify that:
No author identifier is exposed.
No author email is exposed.
No real author name is exposed for anonymous content.
Anonymous display information is returned correctly.
View the same content as the author and confirm ownership is represented via
isOwnPost.Checklist
Sensitive author fields removed from anonymous responses
Backend sanitization implemented
Ownership handled server-side
Tests added
Documentation updated
Existing public-content behavior preserved
Closes #<657>
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation
CodeAnt-AI Description
Prevent anonymous doubts and replies from exposing identity details
What Changed
isOwnPostindicator so the author can still see their own edit/delete controls without exposing identity data.Impact
✅ Fewer anonymous identity leaks✅ Safer delete access for owned doubts✅ Clearer anonymous post privacy💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.