fix: security review by claude#998
Open
MasterIceZ wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds HTTP security headers (CSP, X-Frame-Options, etc.) to the Next.js config for all routes. Enforces authentication on ChangesSecurity Hardening and Access Control
Sequence Diagram(s)sequenceDiagram
participant Client
participant RenderPOST as /api/render POST
participant AssessmentsPOST as /api/assessments POST
participant UsersGET as /api/users GET
participant RealtimeGET as /api/submissions/[id]/realtime GET
participant getServerUser
participant Prisma
rect rgba(100, 150, 255, 0.5)
note over RenderPOST,UsersGET: Core API auth guards
Client->>RenderPOST: POST /api/render {content}
RenderPOST->>getServerUser: getServerUser()
getServerUser-->>RenderPOST: user | null
alt no user
RenderPOST-->>Client: 401 unauthorized()
else authenticated
RenderPOST-->>Client: 200 json(html)
end
end
rect rgba(150, 100, 255, 0.5)
note over AssessmentsPOST,UsersGET: Admin-only endpoints
Client->>AssessmentsPOST: POST /api/assessments
AssessmentsPOST->>getServerUser: getServerUser()
getServerUser-->>AssessmentsPOST: user
alt user.admin false
AssessmentsPOST-->>Client: 403 forbidden()
else admin user
AssessmentsPOST-->>Client: 200 json(assessment)
end
Client->>UsersGET: GET /api/users
UsersGET->>getServerUser: getServerUser()
getServerUser-->>UsersGET: user
alt user.admin false
UsersGET-->>Client: 403 forbidden()
else admin user
UsersGET-->>Client: 200 json(users)
end
end
rect rgba(100, 255, 150, 0.5)
note over RealtimeGET,Prisma: Submission ownership gate
Client->>RealtimeGET: GET /api/submissions/[id]/realtime
RealtimeGET->>Prisma: findUnique({select: {userId, task, ...}})
Prisma-->>RealtimeGET: submission
alt task.private && !admin && userId !== user.id
RealtimeGET-->>Client: 403 forbidden()
else authorized
RealtimeGET->>RealtimeGET: delete payload.task, payload.userId
RealtimeGET-->>Client: 200 json(sanitized)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/render/page.tsx (1)
19-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle thrown fetch errors so loading state is always reset.
On Line 19,
fetchcan throw (network/abort), and the currentres.okbranch won’t run. Wrap this block intry/finallysosetLoading(false)is guaranteed.Suggested fix
async function render() { - setLoading(true) - const res = await fetch('/api/render', { - method: 'POST', - body: JSON.stringify({ content: md }) - }) - if (!res.ok) { - setLoading(false) - return - } - setRendered(await res.json()) - setLoading(false) + setLoading(true) + try { + const res = await fetch('/api/render', { + method: 'POST', + body: JSON.stringify({ content: md }) + }) + if (!res.ok) return + setRendered(await res.json()) + } finally { + setLoading(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/app/render/page.tsx` around lines 19 - 29, The fetch call on line 19 can throw errors due to network issues or request abortion, and when it does, the setLoading(false) statement never executes, leaving the loading state permanently true. Wrap the entire fetch block (from the fetch call through the setRendered and setLoading calls) in a try/finally statement, moving the setLoading(false) call into the finally block to guarantee it always executes regardless of whether fetch throws or returns a response.
🤖 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 `@next.config.mjs`:
- Around line 30-33: Remove the 'unsafe-eval' directive from the script-src
policy in the CSP header array. Locate the line containing "script-src 'self'
'unsafe-inline' 'unsafe-eval'" in next.config.mjs and remove 'unsafe-eval' from
the string, keeping only 'self' and 'unsafe-inline' to maintain necessary
functionality while improving XSS protection.
---
Outside diff comments:
In `@src/app/render/page.tsx`:
- Around line 19-29: The fetch call on line 19 can throw errors due to network
issues or request abortion, and when it does, the setLoading(false) statement
never executes, leaving the loading state permanently true. Wrap the entire
fetch block (from the fetch call through the setRendered and setLoading calls)
in a try/finally statement, moving the setLoading(false) call into the finally
block to guarantee it always executes regardless of whether fetch throws or
returns a response.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10c5f75b-d0b6-4b63-b90c-392bb0263646
📒 Files selected for processing (8)
next.config.mjssrc/app/api/assessments/route.tssrc/app/api/render/route.tssrc/app/api/submissions/[id]/realtime/route.tssrc/app/api/tasks/[id]/route.tssrc/app/api/users/route.tssrc/app/render/page.tsxsrc/lib/api/schema/tasks.ts
💤 Files with no reviewable changes (1)
- src/app/api/tasks/[id]/route.ts
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
1 participant
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.
Summary by CodeRabbit
Bug Fixes
Chores