fix(auth,users): harden OAuth callback and user sync (4 issues)

src/auth/auth.controller.ts

@@ -1,4 +1,14 @@
-import { Controller, Get, Req, Res, UseGuards } from '@nestjs/common';
+import {
+  Body,
+  Controller,
+  Get,
+  HttpCode,
+  Post,
+  Req,
+  Res,
+  UnauthorizedException,
+  UseGuards,
+} from '@nestjs/common';
 import { ConfigService } from '@nestjs/config';
 import { ApiExcludeEndpoint, ApiTags } from '@nestjs/swagger';
 import type { Request, Response } from 'express';
@@ -29,8 +39,50 @@ export class AuthController {
   async githubCallback(@Req() req: Request, @Res() res: Response) {
     const profile = req.user as UpsertFromGithubInput;
     const { accessToken } = await this.authService.loginWithGithub(profile);
+    const code = this.authService.createHandoffCode(accessToken);
+    // Defense-in-depth: also set the JWT as an httpOnly cookie so a frontend
+    // that prefers cookie-based auth never needs to handle a bearer token in
+    // the URL at all. The handoff code remains the primary exchange mechanism.
+    const isProd =
+      this.configService.get('env', { infer: true }) === 'production';
+    res.cookie('access_token', accessToken, {
+      httpOnly: true,
+      secure: isProd,
+      sameSite: 'lax',
+      maxAge: 7 * 24 * 60 * 60 * 1000,
+      path: '/',
+    });
     const frontendUrl = this.configService.get('frontendUrl', { infer: true });
-    res.redirect(`${frontendUrl}/auth/callback?token=${accessToken}`);
+    res.redirect(`${frontendUrl}/auth/callback?code=${code}`);
+  }
+
+  @Post('handoff')
+  @HttpCode(200)
+  @ApiExcludeEndpoint()
+  async exchangeHandoff(@Body('code') code: string) {
+    if (!code || typeof code !== 'string') {
+      throw new UnauthorizedException('Missing handoff code');
+    }
+    const token = this.authService.consumeHandoffCode(code);
+    if (!token) {
+      throw new UnauthorizedException('Invalid or expired handoff code');
+    }
+    return { accessToken: token };
+  }
+
+  @Post('exchange')
+  @HttpCode(200)
+  @ApiExcludeEndpoint()
+  async exchangeHandoffAlias(@Body('code') code: string) {
+    // Alias for POST /auth/exchange — same single-use semantics as /handoff.
+    if (!code || typeof code !== 'string') {
+      throw new UnauthorizedException('Missing handoff code');
+    }
+    const token = this.authService.consumeHandoffCode(code);
+    if (!token) {
+      throw new UnauthorizedException('Invalid or expired handoff code');
+    }
+    return { accessToken: token };
   }
 
   @Get('me')

src/auth/auth.service.ts

@@ -1,10 +1,17 @@
 import { Injectable } from '@nestjs/common';
 import { JwtService } from '@nestjs/jwt';
+import { randomBytes } from 'crypto';
 import { User } from '../common/entities';
 import { UsersService, UpsertFromGithubInput } from '../users/users.service';
 
 @Injectable()
 export class AuthService {
+  private readonly handoffCodes = new Map<
+    string,
+    { token: string; expiresAt: number }
+  >();
+  private readonly HANDOFF_TTL_MS = 5 * 60 * 1000;
+
   constructor(
     private readonly usersService: UsersService,
     private readonly jwtService: JwtService,
@@ -21,4 +28,23 @@ export class AuthService {
   signToken(user: User): string {
     return this.jwtService.sign({ sub: user.id, username: user.username });
   }
+
+  createHandoffCode(token: string): string {
+    const code = randomBytes(32).toString('hex');
+    const expiresAt = Date.now() + this.HANDOFF_TTL_MS;
+    this.handoffCodes.set(code, { token, expiresAt });
+    // Opportunistically prune expired entries to bound memory.
+    for (const [k, v] of this.handoffCodes.entries()) {
+      if (Date.now() > v.expiresAt) this.handoffCodes.delete(k);
+    }
+    return code;
+  }
+
+  consumeHandoffCode(code: string): string | null {
+    const entry = this.handoffCodes.get(code);
+    if (!entry) return null;
+    this.handoffCodes.delete(code);
+    if (Date.now() > entry.expiresAt) return null;
+    return entry.token;
+  }
 }

src/auth/strategies/jwt.strategy.ts

@@ -1,25 +1,54 @@
-import { Injectable } from '@nestjs/common';
+import { Injectable, UnauthorizedException } from '@nestjs/common';
 import { ConfigService } from '@nestjs/config';
 import { PassportStrategy } from '@nestjs/passport';
 import { ExtractJwt, Strategy } from 'passport-jwt';
+import { Request } from 'express';
 import { AppConfig } from '../../config/configuration';
+import { UsersService } from '../../users/users.service';
 
 export interface JwtPayload {
   sub: string;
   username: string;
 }
 
+function cookieExtractor(req: Request): string | null {
+  if (!req) return null;
+  // If cookie-parser is installed, req.cookies will be populated.
+  const cookies = (req as unknown as { cookies?: Record<string, string> })
+    .cookies;
+  if (cookies && cookies['access_token']) return cookies['access_token'];
+  const header = req.headers?.cookie;
+  if (!header) return null;
+  for (const part of header.split(';')) {
+    const [rawKey, ...rest] = part.trim().split('=');
+    if (rawKey === 'access_token') return decodeURIComponent(rest.join('='));
+  }
+  return null;
+}
+
 @Injectable()
 export class JwtStrategy extends PassportStrategy(Strategy) {
-  constructor(configService: ConfigService<AppConfig, true>) {
+  constructor(
+    configService: ConfigService<AppConfig, true>,
+    private readonly usersService: UsersService,
+  ) {
     super({
-      jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(),
+      jwtFromRequest: ExtractJwt.fromExtractors([
+        ExtractJwt.fromAuthHeaderAsBearerToken(),
+        cookieExtractor,
+      ]),
       ignoreExpiration: false,
       secretOrKey: configService.get('jwt', { infer: true }).secret,
     });
   }
 
-  validate(payload: JwtPayload) {
+  async validate(payload: JwtPayload) {
+    if (!payload?.sub) throw new UnauthorizedException('Invalid token payload');
+    try {
+      await this.usersService.findOneRaw(payload.sub);
+    } catch {
+      throw new UnauthorizedException('User no longer exists');
+    }
     return { userId: payload.sub, username: payload.username };
   }
 }

src/escrow/escrow.service.ts

@@ -579,7 +579,7 @@ export class EscrowService {
     return this.soroban.tokenContractId(asset);
   }
 
-  /** Validates that split percentages sum to 100.00, within floating point tolerance. */
+  /**
    * Validates that split percentages sum to 100.00 (within tolerance), with
    * every entry in `(0, 100]`. Delegates to the shared
    * {@link validatePercentageSplits} — the same implementation

src/users/users.service.spec.ts

@@ -104,13 +104,18 @@ describe('UsersService', () => {
 
     it('creates a new User + GithubAccount when neither exists', async () => {
       githubAccountRepo.findOne.mockResolvedValue(null);
-      userRepo.findOne
-        .mockResolvedValueOnce(null) // lookup by username before create
-        .mockResolvedValueOnce({
-          id: 'u1',
-          username: 'octocat',
-          githubAccount: { id: 'ga1' },
-        }); // findOneRaw at the end
+      userRepo.findOne.mockImplementation(async ({ where }: any) => {
+        if (where?.username === 'octocat') return null;
+        if (where?.email === 'octocat@example.com') return null;
+        if (where?.id === 'u1') {
+          return {
+            id: 'u1',
+            username: 'octocat',
+            githubAccount: { id: 'ga1' },
+          };
+        }
+        return null;
+      });
 
       const user = await service.upsertFromGithub(input);
 
@@ -127,17 +132,30 @@ describe('UsersService', () => {
       expect(user.id).toBe('u1');
     });
 
-    it('links to an existing user found by username instead of creating a duplicate', async () => {
+    it('prevents username-based takeover: creates a new user when username is taken by another GitHub identity', async () => {
       githubAccountRepo.findOne.mockResolvedValue(null);
-      userRepo.findOne
-        .mockResolvedValueOnce({ id: 'existing-user', username: 'octocat' })
-        .mockResolvedValueOnce({ id: 'existing-user', username: 'octocat' });
+      // 'octocat' is already owned by Alice; Bob (gh-1) must not be linked to her.
+      userRepo.findOne.mockImplementation(async ({ where }: any) => {
+        if (where?.username === 'octocat')
+          return { id: 'alice-id', username: 'octocat' };
+        if (where?.username === 'octocat-1') return null;
+        if (where?.email === 'octocat@example.com') return null;
+        if (where?.id === 'u1') {
+          return { id: 'u1', username: 'octocat-1', githubAccount: { id: 'ga1' } };
+        }
+        return null;
+      });
 
       await service.upsertFromGithub(input);
 
-      expect(userRepo.create).not.toHaveBeenCalled();
+      expect(userRepo.create).toHaveBeenCalledWith(
+        expect.objectContaining({ username: 'octocat-1' }),
+      );
       expect(githubAccountRepo.create).toHaveBeenCalledWith(
-        expect.objectContaining({ userId: 'existing-user' }),
+        expect.objectContaining({ githubId: 'gh-1', userId: 'u1' }),
+      );
+      expect(githubAccountRepo.create).not.toHaveBeenCalledWith(
+        expect.objectContaining({ userId: 'alice-id' }),
       );
     });
 
@@ -148,9 +166,21 @@ describe('UsersService', () => {
         userId: 'u1',
         accessToken: 'old-token',
         refreshToken: 'old-refresh',
+        login: 'octocat',
       };
       githubAccountRepo.findOne.mockResolvedValue(account);
-      userRepo.findOne.mockResolvedValue({ id: 'u1', username: 'octocat' });
+      userRepo.findOne.mockImplementation(async ({ where }: any) => {
+        if (where?.id === 'u1') {
+          return {
+            id: 'u1',
+            username: 'octocat',
+            displayName: 'The Octocat',
+            avatarUrl: 'https://example.com/a.png',
+            email: 'octocat@example.com',
+          };
+        }
+        return null;
+      });
 
       await service.upsertFromGithub(input);
 
@@ -159,15 +189,72 @@ describe('UsersService', () => {
         expect.objectContaining({
           accessToken: 'token-abc',
           refreshToken: 'refresh-abc',
+          login: 'octocat',
         }),
       );
     });
 
+    it('syncs stale User profile fields from the fresh GitHub profile on re-login', async () => {
+      const account = {
+        id: 'ga1',
+        githubId: 'gh-1',
+        userId: 'u1',
+        accessToken: 'old-token',
+        refreshToken: 'old-refresh',
+        login: 'old-name',
+        avatarUrl: 'https://example.com/old.png',
+        profileUrl: 'https://github.com/old-name',
+      };
+      githubAccountRepo.findOne.mockResolvedValue(account);
+      const staleUser = {
+        id: 'u1',
+        username: 'old-name',
+        displayName: 'Old Name',
+        avatarUrl: 'https://example.com/old.png',
+        email: 'old@example.com',
+      };
+      const updatedUser = {
+        ...staleUser,
+        username: 'octocat',
+        displayName: 'The Octocat',
+        avatarUrl: 'https://example.com/a.png',
+        email: 'octocat@example.com',
+      };
+      // findOneRaw first call returns stale, second call after save returns updated
+      userRepo.findOne
+        .mockResolvedValueOnce(staleUser) // first findOneRaw for sync
+        .mockResolvedValueOnce(null) // check username uniqueness for "octocat"
+        .mockResolvedValueOnce(null) // check email uniqueness
+        .mockResolvedValueOnce(updatedUser); // final findOneRaw
+      userRepo.save.mockImplementation(async (u: any) => u);
+
+      const user = await service.upsertFromGithub(input);
+
+      expect(githubAccountRepo.save).toHaveBeenCalledWith(
+        expect.objectContaining({
+          login: 'octocat',
+          avatarUrl: 'https://example.com/a.png',
+        }),
+      );
+      expect(userRepo.save).toHaveBeenCalledWith(
+        expect.objectContaining({
+          username: 'octocat',
+          displayName: 'The Octocat',
+          avatarUrl: 'https://example.com/a.png',
+          email: 'octocat@example.com',
+        }),
+      );
+      expect(user.username).toBe('octocat');
+    });
+
     it('stores a null refreshToken when GitHub does not return one', async () => {
       githubAccountRepo.findOne.mockResolvedValue(null);
-      userRepo.findOne
-        .mockResolvedValueOnce(null)
-        .mockResolvedValueOnce({ id: 'u1', username: 'octocat' });
+      userRepo.findOne.mockImplementation(async ({ where }: any) => {
+        if (where?.username === 'octocat') return null;
+        if (where?.email === 'octocat@example.com') return null;
+        if (where?.id === 'u1') return { id: 'u1', username: 'octocat' };
+        return null;
+      });
 
       const { refreshToken, ...inputWithoutRefresh } = input;
       void refreshToken;