audit: security hardening, CodeRabbit tuning, guide expansion#27
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
Note
|
| Cohort / File(s) | Summary |
|---|---|
Configuration & Documentation .coderabbit.yaml, CLAUDE.md, README.md |
Updated review profile to assertive with new path filters and security/correctness rules; marked P2.6 Bookmarklet complete; reformatted feature/roadmap tables to reflect Bookmarklet and Collections UI as Live. |
Bookmarklet Feature client/src/app/bookmarklet/page.tsx, client/src/app/bookmarklet/BookmarkletSaver.tsx |
Introduced new bookmarklet page that validates authentication and URL parameters (http/https only), then renders BookmarkletSaver component. Component manages POST request lifecycle with status tracking (saving/saved/duplicate/error) and auto-closes window on success after 1.5s. |
Rate Limiting client/src/lib/ratelimit.ts, client/src/app/api/collections/route.ts, client/src/app/api/collections/[id]/route.ts, client/src/app/api/collections/[id]/bookmarks/route.ts, client/src/app/api/collections/[id]/bookmarks/[bookmarkId]/route.ts, client/src/app/api/bookmarks/[id]/route.ts |
Added collectionRatelimit instance (60 ops/1h sliding window, redis-backed) and applied early-exit rate limit checks to DELETE/POST handlers for collections and bookmarks, returning HTTP 429 when exceeded. |
Validation Enhancements client/src/app/api/bookmarks/[id]/tags/route.ts |
Added tag name length validation; rejects names exceeding 50 characters with HTTP 400 error. |
Landing Page & UI Updates client/src/app/page.tsx, client/src/components/SaveUrlForm.tsx |
Added "Save without leaving the page" marketing section on landing page with 3-column grid describing Web app, Browser extension, and Bookmarklet saving methods; updated SaveUrlForm with draggable bookmarklet link installation UI. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
- M8: Chrome extension — login, save, list, delete bookmarks #19: Modifies the same bookmark DELETE endpoint (
client/src/app/api/bookmarks/[id]/route.ts) with rate limiting and error logging additions. - UI overhaul #20: Modifies the landing page (
client/src/app/page.tsx) with overlapping UI/content updates. - feat: add SaveUrlForm to page for in-browser #4: Updates the same component (
client/src/components/SaveUrlForm.tsx) to include bookmarklet link integration.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the three main changes: security hardening (rate limiting, validation, error fixes), CodeRabbit configuration tuning (.coderabbit.yaml updates), and documentation expansion (GUIDE.md). |
| Description check | ✅ Passed | The description includes a comprehensive summary, categorized issues table with severity and status, test plan checklist, and verification notes. It covers the core requirements, though the template's type-of-change checklist is not explicitly selected. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
audit/security-and-guide-update
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
client/src/app/bookmarklet/page.tsx (1)
46-57: UseisSafeUrl()for stricter URL validation.The current validation accepts both
http:andhttps:protocols, butisSafeUrl()fromclient/src/lib/url-validation.tsenforces HTTPS only and additionally blocks localhost,.localdomains, and private IPs. This provides tighter security at the entry point.Suggested improvement
+import { isSafeUrl } from "@/lib/url-validation"; // Validate URL param let validUrl: string | null = null; if (url) { - try { - const parsed = new URL(url); - if (parsed.protocol === "http:" || parsed.protocol === "https:") { - validUrl = url; - } - } catch { - // invalid + if (isSafeUrl(url)) { + validUrl = url; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/bookmarklet/page.tsx` around lines 46 - 57, Replace the ad-hoc URL parsing logic that sets validUrl with a call to isSafeUrl(url): import isSafeUrl, then set validUrl = url only if isSafeUrl(url) returns true; otherwise leave validUrl null. Remove the try/catch/URL parsing block and ensure you reference the same variables (url and validUrl) so downstream code remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/app/bookmarklet/BookmarkletSaver.tsx`:
- Around line 17-18: The duplicate-detection UI in BookmarkletSaver.tsx expects
a 409 response but api/bookmarks/route.ts currently catches all errors and
always returns 500, so duplicates never surface; update the catch block in the
API route to detect Prisma unique-constraint errors by checking if err is an
instance of Prisma.PrismaClientKnownRequestError and err.code === 'P2002' and
return NextResponse.json({ error: "bookmark already exists" }, { status: 409 })
for that case, otherwise log the error and return the existing 500 response.
In `@client/src/app/bookmarklet/page.tsx`:
- Around line 19-24: The Logo component is currently declared inside the
BookmarkletPage render which triggers an ESLint error and causes component
identity/state to reset on each render; move the Logo function out to module
scope (declare it as a top-level const/function named Logo outside of
BookmarkletPage) and keep using the same <Logo /> JSX in BookmarkletPage,
ensuring the Image import is still in scope and no props change are required.
In `@client/src/app/page.tsx`:
- Around line 399-401: Remove the unnecessary eslint-disable comment before the
anchor in page.tsx: delete the line "/* eslint-disable-next-line no-script-url
*/" that precedes the <a href={`javascript:...`}> so the file no longer contains
an unused eslint directive; keep the existing anchor and its href unchanged
(refer to the anchor/href in page.tsx to locate the spot).
In `@client/src/components/SaveUrlForm.tsx`:
- Around line 71-73: The eslint-disable-next-line no-script-url directive above
the anchor in SaveUrlForm.tsx is unused; remove the comment so the file no
longer contains an unnecessary eslint suppression. Locate the anchor element
setting href to the bookmarklet string in the SaveUrlForm component and delete
the line containing "eslint-disable-next-line no-script-url" (leave the anchor
and its href intact), then run the linter to confirm no warnings remain.
---
Nitpick comments:
In `@client/src/app/bookmarklet/page.tsx`:
- Around line 46-57: Replace the ad-hoc URL parsing logic that sets validUrl
with a call to isSafeUrl(url): import isSafeUrl, then set validUrl = url only if
isSafeUrl(url) returns true; otherwise leave validUrl null. Remove the
try/catch/URL parsing block and ensure you reference the same variables (url and
validUrl) so downstream code remains unchanged.
🪄 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: 4a7aa8a5-fa55-47bb-8c96-4d2bd3e3fc43
📒 Files selected for processing (14)
.coderabbit.yamlCLAUDE.mdREADME.mdclient/src/app/api/bookmarks/[id]/route.tsclient/src/app/api/bookmarks/[id]/tags/route.tsclient/src/app/api/collections/[id]/bookmarks/[bookmarkId]/route.tsclient/src/app/api/collections/[id]/bookmarks/route.tsclient/src/app/api/collections/[id]/route.tsclient/src/app/api/collections/route.tsclient/src/app/bookmarklet/BookmarkletSaver.tsxclient/src/app/bookmarklet/page.tsxclient/src/app/page.tsxclient/src/components/SaveUrlForm.tsxclient/src/lib/ratelimit.ts
| if (res.status === 409) { | ||
| setStatus("duplicate"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if the API route handles duplicates with 409 status
rg -n -A 10 'prisma.bookmark.create' client/src/app/api/bookmarks/route.tsRepository: MinitJain/recall
Length of output: 407
🏁 Script executed:
# Get the complete catch block including return statement(s)
cat -n client/src/app/api/bookmarks/route.ts | sed -n '70,85p'Repository: MinitJain/recall
Length of output: 657
Duplicate detection will not work — API returns 500, not 409.
The component expects HTTP 409 for duplicate URLs (line 17-18), but the API route (lines 70-76) catches all errors and returns HTTP 500. It does not detect Prisma's unique constraint error (P2002) to return 409 for duplicates. The duplicate status will never be triggered.
Fix the API route to detect unique constraint violations:
Proposed fix in client/src/app/api/bookmarks/route.ts
} catch (err) {
if (err instanceof Prisma.PrismaClientKnownRequestError && err.code === 'P2002') {
return NextResponse.json({ error: "bookmark already exists" }, { status: 409 });
}
console.error("Failed to create bookmark:", err);
return NextResponse.json({ error: "failed to save bookmark" }, { status: 500 });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/bookmarklet/BookmarkletSaver.tsx` around lines 17 - 18, The
duplicate-detection UI in BookmarkletSaver.tsx expects a 409 response but
api/bookmarks/route.ts currently catches all errors and always returns 500, so
duplicates never surface; update the catch block in the API route to detect
Prisma unique-constraint errors by checking if err is an instance of
Prisma.PrismaClientKnownRequestError and err.code === 'P2002' and return
NextResponse.json({ error: "bookmark already exists" }, { status: 409 }) for
that case, otherwise log the error and return the existing 500 response.
…ogging - Add collectionRatelimit (60/hr) to all collection POST/DELETE routes - Add bookmarkRatelimit to DELETE /api/bookmarks/[id] (was unprotected) - Add max-length (50 chars) validation on tag name in POST /api/bookmarks/[id]/tags - Add console.error to silent catch in DELETE /api/bookmarks/[id] - Update .coderabbit.yaml: assertive profile, request_changes_workflow, path filters for generated files, and tighter security/Prisma/a11y rules Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
MinitJain
force-pushed
the
audit/security-and-guide-update
branch
from
6c3fdd2 to
642c54e
Compare
1 participant
Summary
What was found and fixed
What was NOT changed
Test plan
Do not merge. Waiting for CodeRabbit review.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation