hard: registration wallet-existence checks are TOCTOU — concurrent registers create duplicate identities

Author: Godfrey-Delight

context/progress-tracker.md

@@ -6,6 +6,7 @@ pure chore/docs commits). Direct pushes to main must also be logged here.
 
 ---
 
+<<<<<<< Updated upstream
 ## 2026-08-24
 
 - **Session families + refresh-token replay detection** (`sessions.family_id`
@@ -26,6 +27,15 @@ pure chore/docs commits). Direct pushes to main must also be logged here.
 - Tests: refresh-family rotation, replay → family-wide revocation + audit
   event, blocked-user denial within TTL bound, cache expiry re-query,
   cleanup job deletes-only-expired.
+=======
+## 2026-08-26
+
+- Fixed registration race conditions in `AuthService.register()` by eliminating application-side pre-checks (`findByWallet`, `checkUsernameExists`) and relying directly on DB-level UNIQUE constraints (`users.wallet_address`, `users.username`).
+- Added idempotent migration `20260826130000_ensure_users_unique_constraints.sql` to ensure unique indexes exist on `users.wallet_address` and `users.username`.
+- Updated `UsersRepository.createProfile()` to catch PostgreSQL unique constraint violation error `23505` and map to structured 409 `ConflictException` (`AUTH_WALLET_EXISTS`, `AUTH_USERNAME_TAKEN`).
+- Added cleanup handlers (`deleteAvatar`, `deleteUserById`) in `AuthService.register()` and `UsersRepository` to ensure failed registrations do not leave orphaned avatar files or partial user records.
+- Added unit tests covering DB unique constraint error mapping, parallel race conditions for duplicate wallet and username registrations, sequential re-registration compatibility, and avatar/user cleanup on failure.
+>>>>>>> Stashed changes
 
 ## 2026-07-23
 

src/database/repositories/users.repository.ts

@@ -1,4 +1,4 @@
-import { Injectable, InternalServerErrorException } from '@nestjs/common';
+import { Injectable, InternalServerErrorException, ConflictException } from '@nestjs/common';
 import { SupabaseService } from '../supabase.client';
 import { UpdateUserDto } from '../../modules/users/dto/update-user.dto';
 
@@ -260,6 +260,19 @@ export class UsersRepository {
             .single();
 
         if (error) {
+            const combinedErr = `${error.code || ''} ${error.message || ''} ${error.details || ''} ${error.hint || ''}`;
+            if (error.code === '23505' || combinedErr.includes('duplicate key') || combinedErr.includes('unique constraint')) {
+                if (combinedErr.includes('username')) {
+                    throw new ConflictException({
+                        code: 'AUTH_USERNAME_TAKEN',
+                        message: 'Username is already taken.',
+                    });
+                }
+                throw new ConflictException({
+                    code: 'AUTH_WALLET_EXISTS',
+                    message: 'Wallet address is already registered.',
+                });
+            }
             throw new InternalServerErrorException({
                 code: 'DATABASE_INSERT_ERROR',
                 message: `Failed to create user profile: ${error.message}`,
@@ -292,4 +305,25 @@ export class UsersRepository {
         const { data } = client.storage.from('avatars').getPublicUrl(fileName);
         return data.publicUrl;
     }
+
+    async deleteAvatar(avatarUrl: string): Promise<void> {
+        try {
+            const fileName = avatarUrl.substring(avatarUrl.lastIndexOf('/') + 1);
+            if (!fileName) return;
+            const client = this.supabaseService.getServiceRoleClient();
+            await client.storage.from('avatars').remove([fileName]);
+        } catch {
+            // Ignore cleanup failures
+        }
+    }
+
+    async deleteUserById(id: string): Promise<void> {
+        try {
+            const client = this.supabaseService.getServiceRoleClient();
+            await client.from('users').delete().eq('id', id);
+        } catch {
+            // Ignore cleanup failures
+        }
+    }
 }
+

src/modules/auth/auth.service.ts

@@ -55,36 +55,42 @@ export class AuthService {
   ) {}
 
   async register(dto: RegisterRequestDto, profileImage?: UploadedAvatarFile): Promise<RegisterResponse> {
-    const existingWallet = await this.usersRepository.findByWallet(dto.walletAddress);
-    if (existingWallet) {
-      throw new ConflictException({ code: 'AUTH_WALLET_EXISTS', message: 'Wallet address is already registered.' });
-    }
-    const usernameTaken = await this.usersRepository.checkUsernameExists(dto.username);
-    if (usernameTaken) {
-      throw new ConflictException({ code: 'AUTH_USERNAME_TAKEN', message: 'Username is already taken.' });
-    }
     let avatarUrl: string | null = null;
-    if (profileImage) {
-      avatarUrl = await this.usersRepository.uploadAvatar(dto.walletAddress, profileImage);
+    let createdUserId: string | null = null;
+    try {
+      if (profileImage) {
+        avatarUrl = await this.usersRepository.uploadAvatar(dto.walletAddress, profileImage);
+      }
+      const user = await this.usersRepository.createProfile({
+        wallet: dto.walletAddress,
+        username: dto.username,
+        displayName: dto.displayName,
+        avatarUrl,
+      });
+      createdUserId = user.id;
+
+      const tokens = await this.generateTokens(dto.walletAddress);
+
+      return {
+        user: {
+          id: user.id,
+          walletAddress: user.wallet_address,
+          username: user.username,
+          displayName: user.display_name,
+          avatarUrl: user.avatar_url,
+          createdAt: user.created_at,
+        },
+        ...tokens,
+      };
+    } catch (error) {
+      if (avatarUrl) {
+        await this.usersRepository.deleteAvatar(avatarUrl).catch(() => {});
+      }
+      if (createdUserId) {
+        await this.usersRepository.deleteUserById(createdUserId).catch(() => {});
+      }
+      throw error;
     }
-    const user = await this.usersRepository.createProfile({
-      wallet: dto.walletAddress,
-      username: dto.username,
-      displayName: dto.displayName,
-      avatarUrl,
-    });
-    const tokens = await this.generateTokens(dto.walletAddress);
-    return {
-      user: {
-        id: user.id,
-        walletAddress: user.wallet_address,
-        username: user.username,
-        displayName: user.display_name,
-        avatarUrl: user.avatar_url,
-        createdAt: user.created_at,
-      },
-      ...tokens,
-    };
   }
 
   async generateNonce(wallet: string): Promise<NonceResponseDto> {

supabase/migrations/20260826130000_ensure_users_unique_constraints.sql

@@ -0,0 +1,4 @@
+-- Ensure DB-level UNIQUE indexes exist on users.wallet_address and users.username
+
+CREATE UNIQUE INDEX IF NOT EXISTS users_wallet_address_idx ON public.users (wallet_address);
+CREATE UNIQUE INDEX IF NOT EXISTS users_username_idx ON public.users (username);

test/unit/modules/auth/auth.service.spec.ts

@@ -42,6 +42,8 @@ describe('AuthService', () => {
     checkUsernameExists: jest.fn(),
     uploadAvatar: jest.fn(),
     createProfile: jest.fn(),
+    deleteAvatar: jest.fn(),
+    deleteUserById: jest.fn(),
   };
 
   const mockAuditService = {
@@ -458,6 +460,8 @@ describe('AuthService', () => {
       mockUsersRepository.checkUsernameExists.mockResolvedValue(false);
       mockUsersRepository.createProfile.mockResolvedValue(mockUser);
       mockUsersRepository.uploadAvatar.mockResolvedValue('https://example.com/avatar.png');
+      mockUsersRepository.deleteAvatar.mockResolvedValue(undefined);
+      mockUsersRepository.deleteUserById.mockResolvedValue(undefined);
 
       // Mock findOrCreateUser internal behavior via Supabase mock
       mockFrom.mockImplementation((table: string) => {
@@ -492,8 +496,6 @@ describe('AuthService', () => {
     it('should register a new user successfully without image', async () => {
       const result = await service.register(registerDto);
 
-      expect(mockUsersRepository.findByWallet).toHaveBeenCalledWith(validWallet);
-      expect(mockUsersRepository.checkUsernameExists).toHaveBeenCalledWith('testuser');
       expect(mockUsersRepository.createProfile).toHaveBeenCalledWith({
         wallet: validWallet,
         username: 'testuser',
@@ -517,23 +519,139 @@ describe('AuthService', () => {
       expect(result.user.avatarUrl).toBe('https://example.com/avatar.png');
     });
 
-    it('should throw ConflictException if wallet already exists', async () => {
-      mockUsersRepository.findByWallet.mockResolvedValue({ id: 'existing' });
+    it('should throw ConflictException (AUTH_WALLET_EXISTS) if DB unique constraint on wallet is violated', async () => {
+      mockUsersRepository.createProfile.mockRejectedValueOnce(
+        new ConflictException({ code: 'AUTH_WALLET_EXISTS', message: 'Wallet address is already registered.' }),
+      );
 
-      await expect(service.register(registerDto)).rejects.toThrow(ConflictException);
       await expect(service.register(registerDto)).rejects.toMatchObject({
         response: { code: 'AUTH_WALLET_EXISTS' },
       });
     });
 
-    it('should throw ConflictException if username is taken', async () => {
-      mockUsersRepository.checkUsernameExists.mockResolvedValue(true);
+    it('should throw ConflictException (AUTH_USERNAME_TAKEN) if DB unique constraint on username is violated', async () => {
+      mockUsersRepository.createProfile.mockRejectedValueOnce(
+        new ConflictException({ code: 'AUTH_USERNAME_TAKEN', message: 'Username is already taken.' }),
+      );
 
-      await expect(service.register(registerDto)).rejects.toThrow(ConflictException);
       await expect(service.register(registerDto)).rejects.toMatchObject({
         response: { code: 'AUTH_USERNAME_TAKEN' },
       });
     });
+
+    it('should handle parallel duplicate-wallet registrations yielding exactly one success and one 409 AUTH_WALLET_EXISTS', async () => {
+      mockUsersRepository.createProfile
+        .mockResolvedValueOnce(mockUser)
+        .mockRejectedValueOnce(
+          new ConflictException({ code: 'AUTH_WALLET_EXISTS', message: 'Wallet address is already registered.' }),
+        );
+
+      const [res1, res2] = await Promise.allSettled([
+        service.register(registerDto),
+        service.register(registerDto),
+      ]);
+
+      const fulfilled = [res1, res2].filter((r) => r.status === 'fulfilled');
+      const rejected = [res1, res2].filter((r) => r.status === 'rejected');
+
+      expect(fulfilled).toHaveLength(1);
+      expect(rejected).toHaveLength(1);
+      if (rejected[0].status === 'rejected') {
+        expect(rejected[0].reason).toBeInstanceOf(ConflictException);
+        expect((rejected[0].reason as ConflictException).getResponse()).toEqual({
+          code: 'AUTH_WALLET_EXISTS',
+          message: 'Wallet address is already registered.',
+        });
+      }
+    });
+
+    it('should handle parallel duplicate-username registrations yielding exactly one success and one 409 AUTH_USERNAME_TAKEN', async () => {
+      const dto2 = { ...registerDto, walletAddress: 'GDIFFERENTWALLETHDHSKDHFKSHDFKSHDFKSHDFKSHDFKSH' };
+
+      mockUsersRepository.createProfile
+        .mockResolvedValueOnce(mockUser)
+        .mockRejectedValueOnce(
+          new ConflictException({ code: 'AUTH_USERNAME_TAKEN', message: 'Username is already taken.' }),
+        );
+
+      const [res1, res2] = await Promise.allSettled([
+        service.register(registerDto),
+        service.register(dto2),
+      ]);
+
+      const fulfilled = [res1, res2].filter((r) => r.status === 'fulfilled');
+      const rejected = [res1, res2].filter((r) => r.status === 'rejected');
+
+      expect(fulfilled).toHaveLength(1);
+      expect(rejected).toHaveLength(1);
+      if (rejected[0].status === 'rejected') {
+        expect(rejected[0].reason).toBeInstanceOf(ConflictException);
+        expect((rejected[0].reason as ConflictException).getResponse()).toEqual({
+          code: 'AUTH_USERNAME_TAKEN',
+          message: 'Username is already taken.',
+        });
+      }
+    });
+
+    it('should return same structured 409 AUTH_WALLET_EXISTS on sequential re-registration', async () => {
+      // First registration succeeds
+      await service.register(registerDto);
+
+      // Second registration fails on unique constraint
+      mockUsersRepository.createProfile.mockRejectedValueOnce(
+        new ConflictException({ code: 'AUTH_WALLET_EXISTS', message: 'Wallet address is already registered.' }),
+      );
+
+      await expect(service.register(registerDto)).rejects.toMatchObject({
+        response: { code: 'AUTH_WALLET_EXISTS' },
+      });
+    });
+
+    it('should clean up avatar from storage when registration fails after avatar upload', async () => {
+      const mockFile = { originalname: 'avatar.png', buffer: Buffer.from('test'), mimetype: 'image/png' };
+      mockUsersRepository.uploadAvatar.mockResolvedValue('https://example.com/avatar.png');
+      mockUsersRepository.createProfile.mockRejectedValueOnce(
+        new ConflictException({ code: 'AUTH_WALLET_EXISTS', message: 'Wallet address is already registered.' }),
+      );
+
+      await expect(service.register(registerDto, mockFile)).rejects.toThrow(ConflictException);
+
+      expect(mockUsersRepository.deleteAvatar).toHaveBeenCalledWith('https://example.com/avatar.png');
+    });
+
+    it('should clean up both avatar and created user if downstream token issuance fails', async () => {
+      const mockFile = { originalname: 'avatar.png', buffer: Buffer.from('test'), mimetype: 'image/png' };
+      mockUsersRepository.uploadAvatar.mockResolvedValue('https://example.com/avatar.png');
+      mockUsersRepository.createProfile.mockResolvedValue(mockUser);
+
+      // Mock session creation failure during generateTokens
+      mockFrom.mockImplementation((table: string) => {
+        if (table === 'users') {
+          return {
+            upsert: jest.fn().mockReturnThis(),
+            select: jest.fn().mockReturnThis(),
+            single: jest.fn().mockResolvedValue({ data: { id: 'user-uuid', status: 'active' }, error: null }),
+          };
+        }
+        if (table === 'learner_profiles') {
+          return {
+            select: jest.fn().mockReturnThis(),
+            eq: jest.fn().mockReturnThis(),
+            maybeSingle: jest.fn().mockResolvedValue({ data: null, error: null }),
+            insert: jest.fn().mockResolvedValue({ error: null }),
+          };
+        }
+        if (table === 'sessions') {
+          return { insert: jest.fn().mockResolvedValue({ error: { message: 'Session failed' } }) };
+        }
+        return { insert: mockInsert };
+      });
+
+      await expect(service.register(registerDto, mockFile)).rejects.toThrow(InternalServerErrorException);
+
+      expect(mockUsersRepository.deleteAvatar).toHaveBeenCalledWith('https://example.com/avatar.png');
+      expect(mockUsersRepository.deleteUserById).toHaveBeenCalledWith('user-uuid');
+    });
   });
 
   // ---------------------------------------------------------------------------