Skip to content

fix: normalize email case to prevent duplicate accounts#287

Open
erquhart wants to merge 3 commits into
mainfrom
fix-email-case-normalization
Open

fix: normalize email case to prevent duplicate accounts#287
erquhart wants to merge 3 commits into
mainfrom
fix-email-case-normalization

Conversation

@erquhart

@erquhart erquhart commented Feb 23, 2026

Copy link
Copy Markdown
Member

Summary

  • Normalizes email addresses (lowercase + trim) on all writes to authAccounts and users, preventing User@Example.com and user@example.com from creating separate accounts
  • Adds a shared findAccountByProviderAndId lookup helper that tries exact match first, then falls back to normalized — backward-compatible with existing mixed-case data
  • Normalizes email comparisons in the Email provider's authorize, the Password provider's defaultProfile, and the rate limit identifier in verifyCodeAndSignIn

Fixes #267

Test plan

  • Added test: password sign-up with mixed case, sign-in with lowercase succeeds
  • Added test: duplicate sign-up with different casing is rejected (single account)
  • Added test: OTP sign-in with different email casing reuses same user/account
  • All 31 existing + new tests pass
  • Build and lint pass

Summary by CodeRabbit

  • New Features

    • Email normalization (lowercase + trim) applied across account creation, sign-in, and verification flows.
  • Bug Fixes

    • Prevents duplicate accounts and sign-in failures caused by email casing or formatting differences; comparisons now tolerate email variations.
  • Tests

    • Added OTP and password tests to ensure sign-ins and sign-ups reuse the same account across email casing/whitespace differences.

@vercel

vercel Bot commented Feb 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
convex-auth-docs Ready Ready Preview Feb 23, 2026 5:26am

Request Review

@coderabbitai

coderabbitai Bot commented Feb 23, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Centralizes provider/account lookup and introduces email normalization (trim + lowercase) across account lookup, creation, verification, and tests to prevent duplicate authAccounts/users caused by email casing or trivial formatting differences.

Changes

Cohort / File(s) Summary
Core Utility
src/server/implementation/utils.ts
Add normalizeEmail(email: string) that trims and lowercases emails for storage/comparison.
Account Lookup
src/server/implementation/mutations/accountLookup.ts
New findAccountByProviderAndId(ctx, provider, providerAccountId) that queries authAccounts by provider+id and retries using normalizeEmail when exact match missing.
Mutation Callsites
src/server/implementation/mutations/createAccountFromCredentials.ts, src/server/implementation/mutations/createVerificationCode.ts, src/server/implementation/mutations/modifyAccount.ts, src/server/implementation/mutations/retrieveAccountWithCredentials.ts
Replace inline provider+accountId index lookups with calls to findAccountByProviderAndId(...); adapt upserts/patches to store normalized providerAccountId where applicable.
Verification & Sign-in Providers
src/providers/Email.ts, src/providers/Password.ts
Use normalizeEmail when deriving or comparing providerAccountId (authorize checks, default profile email).
User / Account Management
src/server/implementation/users.ts
Normalize profile.email on create/update; add uniqueUserWithVerifiedEmail internal lookup (exact then normalized); normalize emailVerified patches when linking accounts.
Tests
test/convex/email.test.ts, test/convex/passwords.test.ts
Add tests verifying OTP and password flows treat differing email casing as the same account (no duplicate users/authAccounts).

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Server
  participant Lookup as AccountLookup
  participant DB
  participant Utils

  Client->>Server: request (create/verify/sign-in) with email X
  Server->>Lookup: findAccountByProviderAndId(provider, X)
  Lookup->>DB: query authAccounts index (provider, X)
  DB-->>Lookup: result
  alt no match
    Lookup->>Utils: normalizeEmail(X)
    Utils-->>Lookup: X_norm
    alt X_norm != X
      Lookup->>DB: query authAccounts index (provider, X_norm)
      DB-->>Lookup: result
    end
  end
  Lookup-->>Server: account or null
  Server->>DB: create/patch/verify using normalized values where applicable
  DB-->>Server: success/failure
  Server-->>Client: response
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibbled caps and trimmed the stray,
Lowered letters so accounts won’t fray.
One email, one home — no duplicates to fear,
Hop, hop, patch applied — carrots near! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: normalizing email case to prevent duplicate accounts, which is the primary objective of the entire PR.
Linked Issues check ✅ Passed All requirements from issue #267 are addressed: email normalization implemented across auth flows (OTP, password), backward compatibility maintained via exact-match-first approach, and duplicate accounts prevented.
Out of Scope Changes check ✅ Passed All changes are directly related to email normalization scope defined in #267; no unrelated refactoring, dependencies, or out-of-scope modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-email-case-normalization

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
src/server/implementation/mutations/accountLookup.ts (1)

17-18: normalizeEmail applied to non-email provider account IDs is semantically misleading.

This function is used by all provider types (OAuth, phone, credentials), not just email. While the behavior is harmless for non-email IDs (they're typically already lowercase, so line 18 returns null early), applying normalizeEmail to a phone number or OAuth subject ID is confusing. Consider either:

  • Renaming to something like normalizeIdentifier, or
  • Adding a guard/comment explaining why this is safe for non-email providers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/implementation/mutations/accountLookup.ts` around lines 17 - 18,
The call to normalizeEmail(providerAccountId) in account lookup is misleading
because providerAccountId can be an OAuth subject, phone number, or credentials
ID; either rename normalizeEmail to a neutral name like normalizeIdentifier (and
update its implementation and all call sites such as in accountLookup.ts) or add
a clear guard/comment near the call explaining why email-normalization is safe
for non-email provider IDs and ensure the function name and signature reflect
its broader purpose (reference: normalizeEmail, providerAccountId,
accountLookup).
src/server/implementation/users.ts (2)

214-214: normalizeEmail is applied unconditionally to all providerAccountId values regardless of provider type.

For email/password providers this is correct, and for phone or numeric OAuth IDs it is currently a no-op. However, custom OAuth/OIDC providers that return mixed-case opaque string sub values (e.g. UUIDs with uppercase or base64 segments) would have their IDs silently lowercased. The write-path normalisation plus the read-path fallback in findAccountByProviderAndId keep new records self-consistent, but the naming is misleading and the blanket application could surprise future contributors adding a new provider type.

Consider guarding the normalization by provider type, or extracting a more generic utility:

♻️ Suggested refinement
- providerAccountId: normalizeEmail(account.providerAccountId),
+ providerAccountId:
+   args.provider.type === "email" || args.provider.type === "credentials"
+     ? normalizeEmail(account.providerAccountId)
+     : account.providerAccountId.trim(),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/implementation/users.ts` at line 214, The code currently applies
normalizeEmail unconditionally to providerAccountId which lowercases/normalizes
opaque provider IDs; update the write path so normalization is conditional: only
call normalizeEmail(providerAccountId) when the provider indicates an
email/password provider (e.g., provider === "email" or providerType ===
"email"), otherwise store providerAccountId as-is (or use a clearly named
generic normalizer if needed). Ensure this change aligns with the read-path
logic in findAccountByProviderAndId and update any tests that assume lowercasing
of non-email provider IDs.

175-185: Same unidirectional backward-compat gap, plus a subtle semantic change for the 2+ case.

Same root cause as in findAccountByProviderAndId: a normalised input (sarah@gmail.com) cannot find an old mixed-case users.email record (Sarah@Gmail.COM) — both old and new code return null here, so it is not a regression, but the migration note from createVerificationCode.ts applies equally here.

There is also a subtle new behaviour: when the exact query returns 2 or more results (ambiguous / corrupt state), the old code returned null immediately, but the new code now falls through and may return a single user found by the normalised email. This implicitly resolves ambiguity by preferring the normalised record. If this is intentional, a short comment explaining the intent would prevent future readers from treating it as a bug.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/implementation/users.ts` around lines 175 - 185, The lookup
currently falls through for users.length !== 1 which causes two issues: it
breaks backward-compat for mixed-case legacy emails and changes behavior for the
ambiguous case (>=2 exact matches) by potentially returning a normalized match
instead of null. Fix by making the function return null immediately when
users.length > 1 to preserve the original ambiguous-case behavior, only proceed
to the normalizeEmail path when users.length === 0, and add a short comment near
this logic (around the users.length check in the function) explaining that we
intentionally return null on multiple exact matches to avoid implicitly
resolving ambiguity by preferring a normalized record.
src/server/implementation/mutations/createVerificationCode.ts (1)

41-41: findAccountByProviderAndId doesn't cover the backward-compat direction for pre-existing mixed-case DB records.

The helper's fallback logic is unidirectional:

  • ✅ Mixed-case input → normalised DB record (finds it via the fallback query)
  • ❌ Normalised input → old mixed-case DB record (early-returns null because normalized === providerAccountId)

Any deployment that has authAccounts.providerAccountId stored in mixed case before this fix is applied will not find those accounts when the client supplies a lowercase email. The old code had the same exact-match-only limitation, so this is not a regression, but the fix is incomplete for existing data.

Consider shipping a one-time migration that normalises all existing providerAccountId (and users.email) values in the database to make the fix effective for all users, not only those who sign up after deployment.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/implementation/mutations/createVerificationCode.ts` at line 41,
The fallback in findAccountByProviderAndId misses the case where the input is
already normalized but DB rows are mixed-case (so normalizedInput ===
providerAccountId returns early and skips the fallback), causing lookups to fail
for pre-existing mixed-case authAccounts.providerAccountId and users.email
values; fix by adding a one-time migration that normalises existing database
values (update authAccounts.providerAccountId and users.email to their canonical
lowercased form), and update findAccountByProviderAndId to be defensive (ensure
it runs the fallback query regardless of the early-equality check or compare
using a case-insensitive DB operation) so both new and legacy records resolve —
reference findAccountByProviderAndId and createVerificationCode.ts when applying
the migration and the defensive lookup change.
test/convex/passwords.test.ts (1)

152-184: Test name is misleading, and a negative credential-validation test is missing.

The name "is rejected" implies the whole operation fails, but expect(tokens).not.toBeNull() shows it actually succeeds (only duplicate account creation is rejected). Consider renaming to something like "duplicate sign up with different email casing returns tokens for existing account".

More importantly, both sign-up attempts use the same password. Adding a case where the second signUp uses a wrong password would confirm that credential validation is not bypassed when the normalized email matches an existing account:

// Second sign-up attempt with wrong password should be rejected
await expect(async () => {
  await t.action(api.auth.signIn, {
    provider: "password",
    params: {
      email: "sarah@gmail.com",
      password: "wrongpassword",
      flow: "signUp",
    },
  });
}).rejects.toThrow("InvalidSecret");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/convex/passwords.test.ts` around lines 152 - 184, Rename the test
"duplicate sign up with different email casing is rejected" to a name that
reflects behavior (e.g., "duplicate sign up with different email casing returns
tokens for existing account") and add a negative credential-validation case:
after the first successful signUp via t.action(api.auth.signIn, params with
"Sarah@Gmail.COM"), call t.action(api.auth.signIn, params with "sarah@gmail.com"
and an incorrect password) and assert it rejects with the expected error (e.g.,
use expect(...).rejects.toThrow("InvalidSecret")) to ensure normalization
doesn’t bypass password validation; keep the existing successful
duplicate-signUp check that asserts tokens are returned and that only one
users/authAccounts row exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/providers/Email.ts`:
- Around line 51-54: Remove the redundant type assertion on
account.providerAccountId in Email.ts: update the conditional using
normalizeEmail(account.providerAccountId as string) to call
normalizeEmail(account.providerAccountId) directly (the account parameter is a
GenericDoc<DataModel, "authAccounts"> and providerAccountId is already typed as
string); keep the rest of the comparison with params.email unchanged and ensure
normalizeEmail is used the same way as in Phone.ts.

---

Nitpick comments:
In `@src/server/implementation/mutations/accountLookup.ts`:
- Around line 17-18: The call to normalizeEmail(providerAccountId) in account
lookup is misleading because providerAccountId can be an OAuth subject, phone
number, or credentials ID; either rename normalizeEmail to a neutral name like
normalizeIdentifier (and update its implementation and all call sites such as in
accountLookup.ts) or add a clear guard/comment near the call explaining why
email-normalization is safe for non-email provider IDs and ensure the function
name and signature reflect its broader purpose (reference: normalizeEmail,
providerAccountId, accountLookup).

In `@src/server/implementation/mutations/createVerificationCode.ts`:
- Line 41: The fallback in findAccountByProviderAndId misses the case where the
input is already normalized but DB rows are mixed-case (so normalizedInput ===
providerAccountId returns early and skips the fallback), causing lookups to fail
for pre-existing mixed-case authAccounts.providerAccountId and users.email
values; fix by adding a one-time migration that normalises existing database
values (update authAccounts.providerAccountId and users.email to their canonical
lowercased form), and update findAccountByProviderAndId to be defensive (ensure
it runs the fallback query regardless of the early-equality check or compare
using a case-insensitive DB operation) so both new and legacy records resolve —
reference findAccountByProviderAndId and createVerificationCode.ts when applying
the migration and the defensive lookup change.

In `@src/server/implementation/users.ts`:
- Line 214: The code currently applies normalizeEmail unconditionally to
providerAccountId which lowercases/normalizes opaque provider IDs; update the
write path so normalization is conditional: only call
normalizeEmail(providerAccountId) when the provider indicates an email/password
provider (e.g., provider === "email" or providerType === "email"), otherwise
store providerAccountId as-is (or use a clearly named generic normalizer if
needed). Ensure this change aligns with the read-path logic in
findAccountByProviderAndId and update any tests that assume lowercasing of
non-email provider IDs.
- Around line 175-185: The lookup currently falls through for users.length !== 1
which causes two issues: it breaks backward-compat for mixed-case legacy emails
and changes behavior for the ambiguous case (>=2 exact matches) by potentially
returning a normalized match instead of null. Fix by making the function return
null immediately when users.length > 1 to preserve the original ambiguous-case
behavior, only proceed to the normalizeEmail path when users.length === 0, and
add a short comment near this logic (around the users.length check in the
function) explaining that we intentionally return null on multiple exact matches
to avoid implicitly resolving ambiguity by preferring a normalized record.

In `@test/convex/passwords.test.ts`:
- Around line 152-184: Rename the test "duplicate sign up with different email
casing is rejected" to a name that reflects behavior (e.g., "duplicate sign up
with different email casing returns tokens for existing account") and add a
negative credential-validation case: after the first successful signUp via
t.action(api.auth.signIn, params with "Sarah@Gmail.COM"), call
t.action(api.auth.signIn, params with "sarah@gmail.com" and an incorrect
password) and assert it rejects with the expected error (e.g., use
expect(...).rejects.toThrow("InvalidSecret")) to ensure normalization doesn’t
bypass password validation; keep the existing successful duplicate-signUp check
that asserts tokens are returned and that only one users/authAccounts row
exists.

Comment thread src/providers/Email.ts
Comment on lines +51 to +54
if (
normalizeEmail(account.providerAccountId as string) !==
normalizeEmail(params.email)
) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n "providerAccountId" --type ts -C 3

Repository: get-convex/convex-auth

Length of output: 11395


🏁 Script executed:

cat -n src/providers/Email.ts | sed -n '40,65p'

Repository: get-convex/convex-auth

Length of output: 945


🏁 Script executed:

cat -n src/providers/Email.ts | head -50

Repository: get-convex/convex-auth

Length of output: 2005


🏁 Script executed:

rg -n "type EmailConfig|interface EmailConfig" src/server/types.ts -A 20

Repository: get-convex/convex-auth

Length of output: 706


🏁 Script executed:

rg -n "account.providerAccountId" src/providers/ -B 2 -A 2

Repository: get-convex/convex-auth

Length of output: 625


Remove the redundant as string cast on account.providerAccountId.

The field is already typed as string by the schema definition (see providerAccountId: v.string() in types.ts). The account parameter is GenericDoc<DataModel, "authAccounts">, which properly infers the field type from the database schema. The Phone provider demonstrates this works without a cast (line 36 of Phone.ts uses it directly without casting). The cast adds no safety benefit and should be removed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/providers/Email.ts` around lines 51 - 54, Remove the redundant type
assertion on account.providerAccountId in Email.ts: update the conditional using
normalizeEmail(account.providerAccountId as string) to call
normalizeEmail(account.providerAccountId) directly (the account parameter is a
GenericDoc<DataModel, "authAccounts"> and providerAccountId is already typed as
string); keep the rest of the comparison with params.email unchanged and ensure
normalizeEmail is used the same way as in Phone.ts.

Comment thread src/server/implementation/mutations/accountLookup.ts
Emails are now lowercased and trimmed on write, and lookups fall back to
normalized form so existing mixed-case accounts remain reachable. This
prevents User@Example.com and user@example.com from creating separate
accounts and users.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@erquhart erquhart force-pushed the fix-email-case-normalization branch from a70f0f7 to f839ddd Compare February 23, 2026 04:03
@pkg-pr-new

pkg-pr-new Bot commented Feb 23, 2026

Copy link
Copy Markdown
npm i https://pkg.pr.new/get-convex/convex-auth/@convex-dev/auth@287

commit: 97e5837

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/server/implementation/users.ts`:
- Line 214: The OAuth sign-in lookup in userOAuth.ts currently queries the
providerAndAccountId index directly and can miss accounts when providerAccountId
was stored normalized (users.ts: providerAccountId: normalizeEmail(...)); update
the lookup in userOAuth.ts (the code that queries providerAndAccountId) to, if
the direct query returns null and normalizeEmail(providerAccountId) differs from
the original providerAccountId, perform a second query using the normalized
value (same fallback logic as in accountLookup.ts lines 15–25) so mixed-case
OIDC sub values match the stored, lowercased account.

In `@test/convex/passwords.test.ts`:
- Around line 328-329: The test named "duplicate sign up with different email
casing is rejected" is misnamed because it actually expects the second signUp
call to succeed and return tokens (i.e., reuse the existing account); rename the
test to reflect intended behavior, e.g., change the test description string in
the test function to "duplicate sign up with different email casing reuses
existing account" (locate the test by the current name and the signUp usage and
the tokens variable) so the name matches the assertion that tokens is non-null.

---

Duplicate comments:
In `@src/providers/Email.ts`:
- Around line 51-54: Remove the redundant type assertion by deleting the
unnecessary "as string" cast on account.providerAccountId in the comparison
inside Email.ts; keep the comparison using
normalizeEmail(account.providerAccountId) !== normalizeEmail(params.email)
(referencing normalizeEmail, account.providerAccountId, and params.email) since
providerAccountId is already typed as string per the schema.

In `@src/server/implementation/mutations/accountLookup.ts`:
- Around line 17-25: The current short-circuit (if normalized ===
providerAccountId return null) prevents falling back to legacy mixed-case stored
values; instead remove that early return and change accountLookup to first query
authAccounts.withIndex("providerAndAccountId") using the normalized value, and
if that returns null then re-query the same index using the original
providerAccountId (to catch legacy mixed-case records); keep using
normalizeEmail, providerAccountId, provider, and the providerAndAccountId index
and return the first non-null unique() result.

Comment thread src/server/implementation/users.ts Outdated
userId,
provider: args.provider.id,
providerAccountId: account.providerAccountId,
providerAccountId: normalizeEmail(account.providerAccountId),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n "providerAndAccountId" --type ts -C 4 src/

Repository: get-convex/convex-auth

Length of output: 3191


🏁 Script executed:

cat -n src/server/implementation/mutations/userOAuth.ts | head -50

Repository: get-convex/convex-auth

Length of output: 2046


userOAuth.ts bypasses normalized account lookup — direct providerAndAccountId query needs fallback for mixed-case OIDC sub values

The OAuth sign-in flow in userOAuth.ts (lines 28–33) queries the providerAndAccountId index directly without attempting a normalized lookup. Since providerAccountId is normalized to lowercase when accounts are created (users.ts line 214), a subsequent sign-in with a mixed-case sub value from OIDC providers will fail to find the stored account, resulting in duplicate account creation or sign-in failure.

The normalized fallback already exists in accountLookup.ts (lines 15–25). Apply the same logic to userOAuth.ts: if the exact query returns null and the normalized form differs, perform a second query on the normalized value.

Major OAuth providers (Google, GitHub) issue numeric sub values, so normalization is a no-op for them. The risk applies to OIDC providers with alphanumeric sub values or any provider returning IDs with uppercase characters.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/implementation/users.ts` at line 214, The OAuth sign-in lookup in
userOAuth.ts currently queries the providerAndAccountId index directly and can
miss accounts when providerAccountId was stored normalized (users.ts:
providerAccountId: normalizeEmail(...)); update the lookup in userOAuth.ts (the
code that queries providerAndAccountId) to, if the direct query returns null and
normalizeEmail(providerAccountId) differs from the original providerAccountId,
perform a second query using the normalized value (same fallback logic as in
accountLookup.ts lines 15–25) so mixed-case OIDC sub values match the stored,
lowercased account.

Comment thread test/convex/passwords.test.ts Outdated
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/server/implementation/mutations/accountLookup.ts (1)

12-16: Consider accepting QueryCtx instead of MutationCtx.

The function only performs reads (ctx.db.query). Widening the parameter to QueryCtx would let this helper be reused from query functions as well, without breaking any current mutation callers (since MutationCtx is structurally assignable to QueryCtx).

♻️ Suggested change
+import { Doc, MutationCtx, QueryCtx } from "../types.js";
-import { Doc, MutationCtx } from "../types.js";
 ...
 export async function findAccountByProviderAndId(
-  ctx: MutationCtx,
+  ctx: QueryCtx,
   provider: string,
   providerAccountId: string,
 ): Promise<Doc<"authAccounts"> | null> {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/implementation/mutations/accountLookup.ts` around lines 12 - 16,
Change the function signature of findAccountByProviderAndId to accept QueryCtx
rather than MutationCtx since it only performs reads; update its parameter type
in the declaration and any related imports/usages (keep callers unchanged
because MutationCtx is structurally compatible), and ensure any references
inside the function that assume mutation-only properties still compile with
QueryCtx (e.g., ctx.db.query remains valid).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/server/implementation/users.ts`:
- Line 219: The OIDC sign-in flow in userOAuth.ts still does a direct
providerAccountId lookup which fails for mixed-case "sub" values; change the
lookup to use the shared findAccountByProviderAndId helper (or at minimum run
normalizeEmail on the incoming providerAccountId before querying) so lookups
match the normalized providerAccountId stored on insert; update the code in
userOAuth.ts to call findAccountByProviderAndId(provider,
normalizeEmail(providerAccountId)) (or replace the direct query that references
providerAccountId) to prevent duplicate accounts.

---

Nitpick comments:
In `@src/server/implementation/mutations/accountLookup.ts`:
- Around line 12-16: Change the function signature of findAccountByProviderAndId
to accept QueryCtx rather than MutationCtx since it only performs reads; update
its parameter type in the declaration and any related imports/usages (keep
callers unchanged because MutationCtx is structurally compatible), and ensure
any references inside the function that assume mutation-only properties still
compile with QueryCtx (e.g., ctx.db.query remains valid).

Move providerAccountId normalization out of createOrUpdateAccount (which
handles all provider types) and into email-specific call sites so OAuth,
phone, and custom credential providers are unaffected.

Also fix misleading test name per review feedback.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/server/implementation/users.ts`:
- Around line 180-190: The current logic lets the ambiguous exact-match case
(users.length === 2) fall through to the normalized lookup and possibly return
an unrelated user; add an early-exit guard so any ambiguous exact-match returns
null. Concretely, in the function that queries into users (the block using
`users` and `normalizeEmail(email)`), change the first check to explicitly
return the single match when `users.length === 1`, but return null immediately
when `users.length > 1` (ambiguous) before computing `normalized` and running
the `normalizedUsers` query; keep the rest of the normalized lookup unchanged.

---

Duplicate comments:
In `@src/server/implementation/users.ts`:
- Around line 214-221: The review comment is a duplicate regarding OAuth
providerAccountId normalization; no code changes are required because the
current insert into "authAccounts" stores account.providerAccountId as-is
(preserving symmetry with userOAuth.ts lookups). Resolve by marking or removing
the duplicate review note and leaving the insert block that writes
providerAccountId, provider, providerAccountId, secret into "authAccounts" in
src/server/implementation/users.ts unchanged.

In `@test/convex/passwords.test.ts`:
- Around line 328-360: The test "duplicate sign up with different email casing
reuses existing account" is correct and requires no code changes: keep the two
signIn actions with "Sarah@Gmail.COM" and "sarah@gmail.com", retain the
assertions that tokens is not null and that ctx.db.query("users").collect() and
ctx.db.query("authAccounts").collect() each have toHaveLength(1) to ensure
deduplication by case-insensitive email matching.

Comment on lines +180 to +190
if (users.length === 1) return users[0];

const normalized = normalizeEmail(email);
if (normalized === email) return null;

const normalizedUsers = await ctx.db
.query("users")
.withIndex("email", (q) => q.eq("email", normalized))
.filter((q) => q.neq(q.field("emailVerificationTime"), undefined))
.take(2);
return normalizedUsers.length === 1 ? normalizedUsers[0] : null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Minor: ambiguous exact match (users.length === 2) silently falls through to the normalized lookup.

The original implementation returned null for any count ≠ 1. The new logic does return users[0] when exactly one exact match exists, but for the users.length === 2 case (two users share the same verbatim email — a data-integrity anomaly), execution falls through to the normalized query. If that second query returns exactly one hit, the function returns that third, unrelated user instead of signalling ambiguity with null. Adding an early-exit guard preserves the intended "ambiguous ⇒ no linking" invariant:

🛡️ Proposed fix
   if (users.length === 1) return users[0];
+  if (users.length > 1) return null;

   const normalized = normalizeEmail(email);
   if (normalized === email) return null;
📝 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
if (users.length === 1) return users[0];
const normalized = normalizeEmail(email);
if (normalized === email) return null;
const normalizedUsers = await ctx.db
.query("users")
.withIndex("email", (q) => q.eq("email", normalized))
.filter((q) => q.neq(q.field("emailVerificationTime"), undefined))
.take(2);
return normalizedUsers.length === 1 ? normalizedUsers[0] : null;
if (users.length === 1) return users[0];
if (users.length > 1) return null;
const normalized = normalizeEmail(email);
if (normalized === email) return null;
const normalizedUsers = await ctx.db
.query("users")
.withIndex("email", (q) => q.eq("email", normalized))
.filter((q) => q.neq(q.field("emailVerificationTime"), undefined))
.take(2);
return normalizedUsers.length === 1 ? normalizedUsers[0] : null;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/implementation/users.ts` around lines 180 - 190, The current logic
lets the ambiguous exact-match case (users.length === 2) fall through to the
normalized lookup and possibly return an unrelated user; add an early-exit guard
so any ambiguous exact-match returns null. Concretely, in the function that
queries into users (the block using `users` and `normalizeEmail(email)`), change
the first check to explicitly return the single match when `users.length === 1`,
but return null immediately when `users.length > 1` (ambiguous) before computing
`normalized` and running the `normalizedUsers` query; keep the rest of the
normalized lookup unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate authAccounts for same email with diferent casing

1 participant