Security: Fix 5 L3 Rate-Limiting, DoS, and OAuth Takeover Vulnerabilities - #523
Security: Fix 5 L3 Rate-Limiting, DoS, and OAuth Takeover Vulnerabilities#523Prathvikmehra wants to merge 6 commits into
Conversation
…ion and safe oauth linking
… on account deletion
|
@Prathvikmehra is attempting to deploy a commit to the vishnukothakapu's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey @vishnukothakapu! I've already tested this entire suite locally. I applied strict rate limits to the 4 exposed routes using the existing lib/rateLimit.ts sliding-window utility and secured the NextAuth config. Let me know when you assign this to me, and I can open the massive PR immediately! |
📝 WalkthroughWalkthroughThe changes add rate limits to several API routes, refactor account merging into a queued Prisma transaction, enforce email verification for credentials login, configure OAuth linking behavior, and use transaction-aware username availability checks during profile publishing. ChangesAPI protection
Account and profile integrity
Authentication policy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant APIRoute
participant RateLimiter
participant ExistingHandler
Client->>APIRoute: Send API request
APIRoute->>RateLimiter: Check email, user, or IP rate-limit key
RateLimiter-->>APIRoute: Return allowance
alt Request allowed
APIRoute->>ExistingHandler: Continue route processing
ExistingHandler-->>Client: Return route response
else Limit exceeded
APIRoute-->>Client: Return HTTP 429
end
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@lib/accountMerge.ts`:
- Around line 143-151: Update completeAccountMerge to reassign every UserAlias
currently owned by sourceUser.id to targetUser.id before prisma.user.delete
executes, not just the current username handled by the existing
userAlias.upsert. Preserve the existing conditional username upsert behavior and
ensure the bulk alias reassignment occurs within the same transaction.
In `@lib/auth.ts`:
- Line 54: Update the GitHub provider configuration in lib/auth.ts to set
allowDangerousEmailAccountLinking to false, matching the Google provider and
keeping unsafe OAuth account linking disabled.
In `@lib/profileWorkflow.ts`:
- Around line 268-273: Update the username-change guard in the profile
publishing workflow to recheck availability only when afterSnapshot.username is
non-null. Preserve the existing beforeSnapshot comparison and error behavior for
actual username values, while skipping isProfileUsernameAvailable when
publishing clears the username.
🪄 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 Plus
Run ID: 90556bd2-40f0-43f2-8d71-b89fd0b8b9ad
📒 Files selected for processing (10)
app/api/export/resume/route.tsapp/api/export/vcard/route.tsapp/api/profile/avatar/route.tsapp/api/profile/merge/complete/route.tsapp/api/profile/merge/request/route.tsapp/api/user/delete/route.tsapp/api/username/check/route.tslib/accountMerge.tslib/auth.tslib/profileWorkflow.ts
| if (shouldAliasSourceUsername && sourceUser.username) { | ||
| transactionOperations.push( | ||
| prisma.userAlias.upsert({ | ||
| where: { username: sourceUser.username }, | ||
| update: { userId: targetUser.id }, | ||
| create: { username: sourceUser.username, userId: targetUser.id }, | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect UserAlias relation + onDelete and related source-owned relations
fd -e prisma --exec sh -c 'echo "== {} =="; cat -n {}'Repository: vishnukothakapu/linkid
Length of output: 9338
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate accountMerge.ts =="
fd -a 'accountMerge.ts' .
echo
echo "== outline accountMerge.ts =="
ast-grep outline lib/accountMerge.ts || true
echo
echo "== relevant lib/accountMerge.ts lines =="
cat -n lib/accountMerge.ts | sed -n '1,280p'
echo
echo "== searches for userAlias relocation / username history ownership =="
rg -n "userAlias|UserAlias|usernameHistory|UsernameHistory|username.*history|updateMany\\(" lib prisma -SRepository: vishnukothakapu/linkid
Length of output: 11525
Reassign source aliases before deleting the source user.
UserAlias.user is onDelete: Cascade, and completeAccountMerge only upserts the source’s current username before issuing prisma.user.delete. Any other aliases already pointing at sourceUser.id are removed with the source user, breaking existing alias URLs. Reassign source-owned aliases to targetUser.id before the delete, or change the relation to non-cascade behavior.
🤖 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 `@lib/accountMerge.ts` around lines 143 - 151, Update completeAccountMerge to
reassign every UserAlias currently owned by sourceUser.id to targetUser.id
before prisma.user.delete executes, not just the current username handled by the
existing userAlias.upsert. Preserve the existing conditional username upsert
behavior and ensure the bulk alias reassignment occurs within the same
transaction.
| GitHub({ | ||
| clientId: process.env.GITHUB_CLIENT_ID, | ||
| clientSecret: process.env.GITHUB_CLIENT_SECRET, | ||
| allowDangerousEmailAccountLinking: true, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked auth files:\n'
fd -a '.*auth.*|auth' . | sed 's#^\./##' | head -50
printf '\nlib/auth.ts outline:\n'
ast-grep outline lib/auth.ts || true
printf '\nRelevant lib/auth.ts lines:\n'
sed -n '1,120p' lib/auth.ts | cat -n
printf '\nSearch allowDangerousEmailAccountLinking:\n'
rg -n "allowDangerousEmailAccountLinking|dangerousEmailAccountLinking|Provider|github" lib src . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: vishnukothakapu/linkid
Length of output: 32504
Disable dangerous email account linking for GitHub.
lib/auth.ts:54 still enables allowDangerousEmailAccountLinking, while the Google provider is set to false. Keep GitHub disabled to avoid re-enabling the unsafe OAuth account linking behavior.
Proposed fix
- allowDangerousEmailAccountLinking: true,
+ allowDangerousEmailAccountLinking: false,📝 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.
| allowDangerousEmailAccountLinking: true, | |
| allowDangerousEmailAccountLinking: 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 `@lib/auth.ts` at line 54, Update the GitHub provider configuration in
lib/auth.ts to set allowDangerousEmailAccountLinking to false, matching the
Google provider and keeping unsafe OAuth account linking disabled.
| if (beforeSnapshot.username && beforeSnapshot.username !== afterSnapshot.username) { | ||
| // Recheck availability within the transaction to guard against TOCTOU races | ||
| const takenByOther = await tx.user.findFirst({ | ||
| where: { username: afterSnapshot.username, NOT: { id: userId } }, | ||
| }); | ||
| if (takenByOther) { | ||
| const isAvailable = await isProfileUsernameAvailable(afterSnapshot.username, userId, tx); | ||
| if (!isAvailable) { | ||
| throw new Error("Username already taken"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm ProfileSnapshot.username nullability and helper signature
rg -nP 'ProfileSnapshot' lib/profileWorkflow.ts -A12
rg -nP 'isProfileUsernameAvailable' lib/profileWorkflow.ts -A6Repository: vishnukothakapu/linkid
Length of output: 7745
🏁 Script executed:
#!/bin/bash
# Inspect the remaining relevant implementation and Prisma schema field nullability.
sed -n '192,216p' lib/profileWorkflow.ts | cat -n
echo '--- schema username field ---'
rg -n 'model User|profileDraft|username' prisma 2>/dev/null || trueRepository: vishnukothakapu/linkid
Length of output: 2104
Short-circuit availability checking when publishing clears the username.
The guard enters when beforeSnapshot.username is non-null and different from afterSnapshot.username, but publishing with afterSnapshot.username === null still calls isProfileUsernameAvailable(null, ...) even though the helper expects string. Only run the availability re-check when the resulting username is non-null.
Proposed guard
- if (beforeSnapshot.username && beforeSnapshot.username !== afterSnapshot.username) {
+ if (
+ beforeSnapshot.username &&
+ afterSnapshot.username &&
+ beforeSnapshot.username !== afterSnapshot.username
+ ) {
// Recheck availability within the transaction to guard against TOCTOU races
const isAvailable = await isProfileUsernameAvailable(afterSnapshot.username, userId, tx);📝 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.
| if (beforeSnapshot.username && beforeSnapshot.username !== afterSnapshot.username) { | |
| // Recheck availability within the transaction to guard against TOCTOU races | |
| const takenByOther = await tx.user.findFirst({ | |
| where: { username: afterSnapshot.username, NOT: { id: userId } }, | |
| }); | |
| if (takenByOther) { | |
| const isAvailable = await isProfileUsernameAvailable(afterSnapshot.username, userId, tx); | |
| if (!isAvailable) { | |
| throw new Error("Username already taken"); | |
| } | |
| if ( | |
| beforeSnapshot.username && | |
| afterSnapshot.username && | |
| beforeSnapshot.username !== afterSnapshot.username | |
| ) { | |
| // Recheck availability within the transaction to guard against TOCTOU races | |
| const isAvailable = await isProfileUsernameAvailable(afterSnapshot.username, userId, tx); | |
| if (!isAvailable) { | |
| throw new Error("Username already taken"); | |
| } |
🤖 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 `@lib/profileWorkflow.ts` around lines 268 - 273, Update the username-change
guard in the profile publishing workflow to recheck availability only when
afterSnapshot.username is non-null. Preserve the existing beforeSnapshot
comparison and error behavior for actual username values, while skipping
isProfileUsernameAvailable when publishing clears the username.
1 participant
Resolves #524
🐛 Bug Description
I conducted a holistic architectural review of the platform's API surface and discovered a cluster of 5 distinct L3 Security and Architectural Vulnerabilities.
Specifically, multiple highly sensitive endpoints lack fundamental rate-limiting controls, leaving the Node.js event loop, database connection pool, and cloud storage vulnerable to Layer-7 Denial of Service (DoS) and brute-force oracle attacks. Furthermore, I identified a silent account takeover vector in the OAuth configuration.
Vulnerability Breakdown: