fix(username): normalize username handling to lowercase#240
Conversation
|
@Aditya30ag is attempting to deploy a commit to the vishnukothakapu's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds normalizeUsername (trim + lowercase), updates validateUsername to operate on normalized input and enforce lowercase, applies normalization in username check/create API handlers and profile workflow, adds tests for validation/normalization, and includes the new test file in the npm test script. ChangesUsername Normalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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)
Comment |
|
I had also test it and all tests passes!! |
![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 (2)
lib/profileWorkflow.ts (1)
220-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecheck
userAliasin the publish transaction guard.The TOCTOU guard currently checks
tx.useronly. A concurrentUserAliasinsert for the same username can still slip through and violate the user+alias uniqueness rule.Proposed fix
- const takenByOther = await tx.user.findFirst({ - where: { username: afterSnapshot.username, NOT: { id: userId } }, - }); - if (takenByOther) { + const [takenByOtherUser, takenByOtherAlias] = await Promise.all([ + tx.user.findFirst({ + where: { username: afterSnapshot.username, NOT: { id: userId } }, + }), + tx.userAlias.findFirst({ + where: { username: afterSnapshot.username, NOT: { userId } }, + }), + ]); + if (takenByOtherUser || takenByOtherAlias) { 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 220 - 236, The TOCTOU guard that rechecks username availability uses only tx.user.findFirst; also re-check tx.userAlias inside the same transaction to prevent a concurrent UserAlias insert from colliding. Inside the username-change branch (where beforeSnapshot.username !== afterSnapshot.username) add a transactional query like tx.userAlias.findFirst({ where: { username: afterSnapshot.username, NOT: { userId } } }) and throw the same "Username already taken" error if it exists (ensuring you exclude any alias already owned by this user); keep this check before you proceed to create the alias.app/api/username/create/route.ts (1)
41-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not return the full
Userrecord from this endpoint.
prisma.user.updatewithoutselectcan include sensitive columns (notablypassword), which should never be serialized in API responses.Proposed fix
const user = await prisma.user.update({ where: { id: userId }, data: { username: normalizedUsername }, + select: { + id: true, + username: true, + name: true, + bio: true, + image: true, + }, });🤖 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 `@app/api/username/create/route.ts` around lines 41 - 47, The endpoint currently returns the full User object from prisma.user.update (in route.ts), which can include sensitive fields like password; modify the prisma.user.update call to include an explicit select clause (e.g., select: { id: true, username: true, email: true, createdAt: true } or whatever safe subset you need) so only non-sensitive fields are returned, assign that result to the user variable, and then return NextResponse.json({ success: true, user }, { status: 200 }) as before; do not return the raw update result without select.
🤖 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 `@app/api/username/check/route.ts`:
- Around line 19-43: The route currently treats any normalizedUsername as valid
and generates suggestions that may not pass the shared validateUsername check;
update the handler to first run validateUsername(normalizedUsername) and return
available: false if it fails, and also filter the suggestions array (the
candidates construction using abbr, `${normalizedUsername}.dev`,
`the${normalizedUsername}`, etc.) through validateUsername before de-duplicating
and returning them; keep the existing isAvailable call but only use it for
inputs that passed validateUsername and only propose candidates that pass
validateUsername as well.
---
Outside diff comments:
In `@app/api/username/create/route.ts`:
- Around line 41-47: The endpoint currently returns the full User object from
prisma.user.update (in route.ts), which can include sensitive fields like
password; modify the prisma.user.update call to include an explicit select
clause (e.g., select: { id: true, username: true, email: true, createdAt: true }
or whatever safe subset you need) so only non-sensitive fields are returned,
assign that result to the user variable, and then return NextResponse.json({
success: true, user }, { status: 200 }) as before; do not return the raw update
result without select.
In `@lib/profileWorkflow.ts`:
- Around line 220-236: The TOCTOU guard that rechecks username availability uses
only tx.user.findFirst; also re-check tx.userAlias inside the same transaction
to prevent a concurrent UserAlias insert from colliding. Inside the
username-change branch (where beforeSnapshot.username !==
afterSnapshot.username) add a transactional query like tx.userAlias.findFirst({
where: { username: afterSnapshot.username, NOT: { userId } } }) and throw the
same "Username already taken" error if it exists (ensuring you exclude any alias
already owned by this user); keep this check before you proceed to create the
alias.
🪄 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: fd2fa135-9fe7-46bc-8f17-9e29ba363abb
📒 Files selected for processing (6)
app/api/username/check/route.tsapp/api/username/create/route.tslib/profileWorkflow.tslib/validations/username.test.tslib/validations/username.tspackage.json
| const normalizedUsername = normalizeUsername(username); | ||
|
|
||
| const validation = validateUsername(username); | ||
| const validation = validateUsername(normalizedUsername); |
There was a problem hiding this comment.
[validateUsername] currently allows uppercase; prefer enforcing a canonical rule (e.g. lowercase only) and normalize input before calling [validateUsername] so checks are consistent.
| return { valid: false, error: "Username must be at least 3 characters" }; | ||
| } | ||
| if (username.length > 30) { | ||
| if (normalizedUsername.length > 30) { |
There was a problem hiding this comment.
The max-length error says "under 30 characters" but the code uses > 30. Make message precise ("at most 30 characters") to avoid confusion.
|
Hello @Anushreebasics usernames are now normalized to lowercase before validation and persistence |
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)
lib/validations/username.ts (1)
1-1: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAlign
usernameRegexwith lowercase-only policy.
usernameRegexstill permits uppercase (A-Z), which conflicts with the lowercase-only rule and error text. Tightening the regex keeps the rule self-consistent if this exported regex is reused elsewhere.Suggested diff
-export const usernameRegex = /^[a-zA-Z0-9-]+$/; +export const usernameRegex = /^[a-z0-9-]+$/;Also applies to: 19-23
🤖 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/validations/username.ts` at line 1, usernameRegex currently allows uppercase letters; update the exported usernameRegex to enforce lowercase-only characters (use a pattern that permits only a–z, 0–9 and hyphen, e.g. replace the A-Z character class with a-z) so it matches the lowercase-only policy and error text; update any other occurrences of usernameRegex in the file (the repeated definitions around lines 19–23) to the same lowercase-only pattern to keep behavior consistent across exports.
🤖 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/validations/username.test.ts`:
- Around line 10-12: The test title "validateUsername accepts mixed case input
after normalization" is misleading because the assertion calls
validateUsername("linkid-user") (all lowercase); rename the test description to
accurately reflect the assertion (e.g., "validateUsername accepts lowercase
input") or update the assertion to use a mixed-case input if you intended to
test normalization; locate the test that calls validateUsername in
username.test.ts and update the string passed to test(...) or the
validateUsername argument accordingly.
---
Outside diff comments:
In `@lib/validations/username.ts`:
- Line 1: usernameRegex currently allows uppercase letters; update the exported
usernameRegex to enforce lowercase-only characters (use a pattern that permits
only a–z, 0–9 and hyphen, e.g. replace the A-Z character class with a-z) so it
matches the lowercase-only policy and error text; update any other occurrences
of usernameRegex in the file (the repeated definitions around lines 19–23) to
the same lowercase-only pattern to keep behavior consistent across exports.
🪄 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: cb407fde-1ee2-4a63-addd-3d477419e094
📒 Files selected for processing (2)
lib/validations/username.test.tslib/validations/username.ts
| @@ -1,4 +1,5 @@ | |||
| import prisma from "@/lib/prisma"; | |||
| import { normalizeUsername } from "@/lib/validations/username"; | |||
There was a problem hiding this comment.
Suggestions like ${username}.dev include dots and will fail validation—filter candidates using [validateUsername] before returning.
| test("validateUsername rejects uppercase usernames", () => { | ||
| assert.equal(validateUsername("LinkID-User").valid, false); | ||
| assert.equal(validateUsername(" LINKID-USER ").valid, false); | ||
| }); No newline at end of file |
There was a problem hiding this comment.
add a new line at the end of file
| import prisma from "@/lib/prisma"; | ||
| import { normalizeUsername } from "@/lib/validations/username"; | ||
| import { NextResponse } from "next/server"; | ||
|
|
There was a problem hiding this comment.
Consider rate-limiting/caching this endpoint to prevent username enumeration.
| `${normalizedUsername}.${year}`, | ||
| `${normalizedUsername}${rand}`, | ||
| ].filter(Boolean) as string[])]; | ||
|
|
There was a problem hiding this comment.
Don’t return the full Prisma [user] object—return a minimal safe projection (e.g., [id], [username], [name])
| `${normalizedUsername}${rand}`, | ||
| ].filter(Boolean) as string[])]; | ||
|
|
||
| const suggestions: string[] = []; |
There was a problem hiding this comment.
Avoid TOCTOU race by performing update + alias creation inside a transaction (or restrict endpoint to initial username set).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@Anushreebasics |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/api/username/check/route.ts (1)
63-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMain username is not validated before reporting availability.
The endpoint can return
available: truefor usernames containing invalid characters (e.g.,AB!@#$CD→ab!@#$cd). The create endpoint would then reject this with a validation error, breaking the check→create flow.Add validation after normalization:
🐛 Proposed fix
if (!normalizedUsername) { return jsonResponse({ available: false, suggestions: [] }); } + + const baseValidation = validateUsername(normalizedUsername); + if (!baseValidation.valid) { + return jsonResponse({ available: false, suggestions: [] }); + } const available = await isAvailable(normalizedUsername);🤖 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 `@app/api/username/check/route.ts` around lines 63 - 71, After normalization, the code must validate the normalizedUsername before calling isAvailable or returning available: true; add a check (using the existing username validation function if one exists—e.g., validateUsername or isValidUsername—or apply the same regex/rules used by the create flow) in route.ts where normalizedUsername is set and before the isAvailable call, and if invalid return jsonResponse({ available: false, suggestions: [] }); so the check endpoint mirrors create's validation and never reports true for invalid usernames.
🧹 Nitpick comments (1)
app/api/username/check/route.ts (1)
5-43: ⚡ Quick winRate limit state lacks cleanup for expired entries.
The
rateLimitStateMap accumulates entries for every unique client IP but never removes them after they expire. In long-running server deployments this can grow unbounded.Consider pruning stale entries periodically or switching to an LRU-based cache:
♻️ Suggested cleanup approach
+function pruneExpiredEntries() { + const now = Date.now(); + for (const [key, value] of rateLimitState) { + if (value.resetAt <= now) { + rateLimitState.delete(key); + } + } +} + function isRateLimited(request: Request): boolean { const key = getClientKey(request); const now = Date.now(); + // Prune occasionally (e.g., every 100 calls or via a time check) + if (rateLimitState.size > 1000) { + pruneExpiredEntries(); + } const current = rateLimitState.get(key);🤖 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 `@app/api/username/check/route.ts` around lines 5 - 43, The rateLimitState Map (used by isRateLimited and keyed via getClientKey) never removes expired entries which can grow unbounded; update the implementation to prune stale entries (entries whose resetAt <= Date.now())—either by (a) running a periodic cleanup task that iterates rateLimitState and deletes expired keys, or (b) performing an in-line cleanup pass in isRateLimited before using the map; ensure the cleanup references RATE_LIMIT_WINDOW_MS and RATE_LIMIT_MAX_REQUESTS consistently and preserves the existing logic that creates/updates the per-key {count, resetAt} objects.
🤖 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.
Duplicate comments:
In `@app/api/username/check/route.ts`:
- Around line 63-71: After normalization, the code must validate the
normalizedUsername before calling isAvailable or returning available: true; add
a check (using the existing username validation function if one exists—e.g.,
validateUsername or isValidUsername—or apply the same regex/rules used by the
create flow) in route.ts where normalizedUsername is set and before the
isAvailable call, and if invalid return jsonResponse({ available: false,
suggestions: [] }); so the check endpoint mirrors create's validation and never
reports true for invalid usernames.
---
Nitpick comments:
In `@app/api/username/check/route.ts`:
- Around line 5-43: The rateLimitState Map (used by isRateLimited and keyed via
getClientKey) never removes expired entries which can grow unbounded; update the
implementation to prune stale entries (entries whose resetAt <=
Date.now())—either by (a) running a periodic cleanup task that iterates
rateLimitState and deletes expired keys, or (b) performing an in-line cleanup
pass in isRateLimited before using the map; ensure the cleanup references
RATE_LIMIT_WINDOW_MS and RATE_LIMIT_MAX_REQUESTS consistently and preserves the
existing logic that creates/updates the per-key {count, resetAt} objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0394efa5-6ade-4b52-b291-a01f2841e0aa
📒 Files selected for processing (3)
app/api/username/check/route.tsapp/api/username/create/route.tslib/validations/username.test.ts
✅ Files skipped from review due to trivial changes (1)
- lib/validations/username.test.ts
| `${short}${year}`, | ||
| `${username}.${year}`, | ||
| `${username}${rand}`, | ||
| `${normalizedUsername}.${year}`, |
There was a problem hiding this comment.
this file returns candidates with dots (e.g. .dev, [.${year}]) that violate [validateUsername] — filter candidates with the validator.
| prisma.user.findUnique({ where: { username } }), | ||
| prisma.userAlias.findUnique({ where: { username } }), | ||
| ]); | ||
| const user = await prisma.$transaction(async (tx) => { |
There was a problem hiding this comment.
returns the full Prisma [user] — return a minimal projection (id, username, name).
| const normalizedUsername = normalizeUsername(username); | ||
|
|
||
| const validation = validateUsername(username); | ||
| const validation = validateUsername(normalizedUsername); |
There was a problem hiding this comment.
updates username without transaction/alias creation. Either restrict to initial set or perform update + alias creation inside a transaction (see [publishProfileDraft] behaviour)
| if (data.username) { | ||
| const basicValidation = validateUsername(data.username); | ||
| const normalizedUsername = normalizeUsername(data.username); | ||
| const basicValidation = validateUsername(normalizedUsername); |
There was a problem hiding this comment.
[rollbackProfileVersion]does not re-check username availability before applying historical usernames — add availability check and alias handling.
Anushreebasics
left a comment
Anushreebasics
left a comment
There was a problem hiding this comment.
Normalize inputs ([.trim().toLowerCase()]) consistently before validation in both check and create endpoints.
|
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 `@lib/profileWorkflow.ts`:
- Around line 430-450: The outer condition currently skips the
username-availability recheck when beforeSnapshot.username is falsy; change the
logic to run the rollback branch whenever beforeSnapshot.username !==
afterSnapshot.username, and inside that branch perform the collision checks only
when afterSnapshot.username is present (normalize via normalizeUsername and call
tx.user.findFirst / tx.userAlias.findFirst to detect conflicts before assigning
afterSnapshot.username), while only calling ensureUsernameAliases(tx, userId,
beforeSnapshot.username) when beforeSnapshot.username is present; update the
block around beforeSnapshot/afterSnapshot handling and references to
normalizeUsername, tx.user.findFirst, tx.userAlias.findFirst,
afterSnapshot.username, beforeSnapshot.username, ensureUsernameAliases, and
userId accordingly.
🪄 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: faf0db21-7106-4d90-8f93-3c20193eefae
📒 Files selected for processing (1)
lib/profileWorkflow.ts
| if (beforeSnapshot.username && beforeSnapshot.username !== afterSnapshot.username) { | ||
| if (afterSnapshot.username) { | ||
| const normalizedUsername = normalizeUsername(afterSnapshot.username); | ||
| const takenByOther = await tx.user.findFirst({ | ||
| where: { username: normalizedUsername, NOT: { id: userId } }, | ||
| select: { id: true }, | ||
| }); | ||
|
|
||
| const aliasTakenByOther = await tx.userAlias.findFirst({ | ||
| where: { username: normalizedUsername, NOT: { userId } }, | ||
| select: { id: true }, | ||
| }); | ||
|
|
||
| if (takenByOther || aliasTakenByOther) { | ||
| throw new Error("Username already taken"); | ||
| } | ||
|
|
||
| afterSnapshot.username = normalizedUsername; | ||
| } | ||
|
|
||
| await ensureUsernameAliases(tx, userId, beforeSnapshot.username); |
There was a problem hiding this comment.
Don't skip rollback collision checks on null → username restores.
Line 430 still gates the new availability recheck on beforeSnapshot.username. If the current profile has no username and the historical version does, rollback skips the user/userAlias check entirely and can restore a username that's now reserved by someone else.
Suggested fix
- if (beforeSnapshot.username && beforeSnapshot.username !== afterSnapshot.username) {
+ if (beforeSnapshot.username !== afterSnapshot.username) {
if (afterSnapshot.username) {
const normalizedUsername = normalizeUsername(afterSnapshot.username);
const takenByOther = await tx.user.findFirst({
where: { username: normalizedUsername, NOT: { id: userId } },
select: { id: true },
});
const aliasTakenByOther = await tx.userAlias.findFirst({
where: { username: normalizedUsername, NOT: { userId } },
select: { id: true },
});
if (takenByOther || aliasTakenByOther) {
throw new Error("Username already taken");
}
afterSnapshot.username = normalizedUsername;
}
- await ensureUsernameAliases(tx, userId, beforeSnapshot.username);
+ if (beforeSnapshot.username) {
+ await ensureUsernameAliases(tx, userId, beforeSnapshot.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 `@lib/profileWorkflow.ts` around lines 430 - 450, The outer condition currently
skips the username-availability recheck when beforeSnapshot.username is falsy;
change the logic to run the rollback branch whenever beforeSnapshot.username !==
afterSnapshot.username, and inside that branch perform the collision checks only
when afterSnapshot.username is present (normalize via normalizeUsername and call
tx.user.findFirst / tx.userAlias.findFirst to detect conflicts before assigning
afterSnapshot.username), while only calling ensureUsernameAliases(tx, userId,
beforeSnapshot.username) when beforeSnapshot.username is present; update the
block around beforeSnapshot/afterSnapshot handling and references to
normalizeUsername, tx.user.findFirst, tx.userAlias.findFirst,
afterSnapshot.username, beforeSnapshot.username, ensureUsernameAliases, and
userId accordingly.
Anushreebasics
left a comment
Anushreebasics
left a comment
There was a problem hiding this comment.
There are still some issues that were suggested but were not resolved, pls complete all the suggestions completely, then only ping for reviewing:
[route.ts]: suggestion generation still returns invalid usernames like ones with dots, so some suggestions can never be accepted.
[route.ts]: username input is still not normalized consistently, and the endpoint still returns the full Prisma user object.
[route.ts]: username change flow still does not create an alias for the previous username or do the update in a transaction.
[profileWorkflow.ts]: rollback still applies old usernames without rechecking availability, so conflicts can still happen.
[username.ts]: the length error text is still a bit inaccurate.
3 participants
What this PR does
Fixes inconsistent username handling caused by case-sensitivity mismatches between username availability checks and username creation.
Changes made
Problem solved
Previously, usernames like
Johnandjohncould behave inconsistently because availability checks were case-insensitive while storage remained case-sensitive.This PR ensures username validation, storage, availability checks, and profile URLs all follow the same lowercase normalization behavior.
Issue #239
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores