From 62762c1be513521cd79b429fd205a6465d81e7f4 Mon Sep 17 00:00:00 2001 From: Darkvader-ship-it Date: Thu, 27 Aug 2026 01:06:42 +0100 Subject: [PATCH] fix(auth,users): harden OAuth callback and user sync (4 issues) - auth: replace JWT-in-URL with single-use handoff code + httpOnly cookie * AuthService.createHandoffCode/consumeHandoffCode stores 5m TTL opaque code * githubCallback now sets httpOnly access_token cookie and redirects with ?code= * POST /auth/handoff and /auth/exchange exchange code for JWT (single-use) - auth: jwt strategy validates user existence on every request * JwtStrategy now injects UsersService and calls findOneRaw(sub) * throws UnauthorizedException when user row no longer exists * supports Bearer header and httpOnly cookie extraction - users: sync User row on re-login to prevent staleness * upsertFromGithub now updates username/displayName/avatarUrl/email from fresh GitHub profile when GithubAccount already exists * respects username/email uniqueness - users: prevent username-recycling takeover * upsertFromGithub never reuses a User by username when githubId is new * always creates a fresh User; collision on username generates a unique variant via generateUniqueUsername - escrow: fix duplicate JSDoc that broke tsc parsing --- src/auth/auth.controller.ts | 56 ++++++++++++- src/auth/auth.service.ts | 26 ++++++ src/auth/strategies/jwt.strategy.ts | 37 ++++++++- src/escrow/escrow.service.ts | 2 +- src/users/users.service.spec.ts | 121 ++++++++++++++++++++++++---- src/users/users.service.ts | 93 ++++++++++++++++++--- 6 files changed, 301 insertions(+), 34 deletions(-) diff --git a/src/auth/auth.controller.ts b/src/auth/auth.controller.ts index ba30147..24ef68f 100644 --- a/src/auth/auth.controller.ts +++ b/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') diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index b0e8036..f03904f 100644 --- a/src/auth/auth.service.ts +++ b/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; + } } diff --git a/src/auth/strategies/jwt.strategy.ts b/src/auth/strategies/jwt.strategy.ts index 2deaeea..fe054d9 100644 --- a/src/auth/strategies/jwt.strategy.ts +++ b/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 }) + .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) { + constructor( + configService: ConfigService, + 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 }; } } diff --git a/src/escrow/escrow.service.ts b/src/escrow/escrow.service.ts index 07be75a..acefa4e 100644 --- a/src/escrow/escrow.service.ts +++ b/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 diff --git a/src/users/users.service.spec.ts b/src/users/users.service.spec.ts index d427c5c..9028995 100644 --- a/src/users/users.service.spec.ts +++ b/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; diff --git a/src/users/users.service.ts b/src/users/users.service.ts index 338589e..c95542b 100644 --- a/src/users/users.service.ts +++ b/src/users/users.service.ts @@ -55,24 +55,83 @@ export class UsersService { account.refreshToken = input.refreshToken ?? null; account.avatarUrl = input.avatarUrl; account.profileUrl = input.profileUrl; + account.login = input.login; await this.githubAccountRepo.save(account); + + // Keep the parent User row in sync with the fresh GitHub profile so + // bounty listings / dashboards do not display stale username/avatar data. + const user = await this.findOneRaw(account.userId); + let needsSave = false; + + if (user.username !== input.login) { + const existingByUsername = await this.userRepo.findOne({ + where: { username: input.login }, + }); + if (!existingByUsername || existingByUsername.id === user.id) { + user.username = input.login; + needsSave = true; + } + } + if (user.displayName !== input.displayName) { + user.displayName = input.displayName; + needsSave = true; + } + if (user.avatarUrl !== input.avatarUrl) { + user.avatarUrl = input.avatarUrl; + needsSave = true; + } + if (user.email !== input.email) { + if (input.email === null) { + user.email = null; + needsSave = true; + } else { + const existingByEmail = await this.userRepo.findOne({ + where: { email: input.email }, + }); + if (!existingByEmail || existingByEmail.id === user.id) { + user.email = input.email; + needsSave = true; + } + } + } + if (needsSave) { + await this.userRepo.save(user); + } return this.findOneRaw(account.userId); } - let user = await this.userRepo.findOne({ - where: { username: input.login }, + // No GithubAccount for this githubId — this is a new GitHub identity. + // Never fall back to a username lookup (GitHub usernames are recyclable; + // reusing a User row by username would allow account takeover). Always + // create a fresh User, handling username collisions by generating a + // unique variant. + let username = input.login; + const existingUsername = await this.userRepo.findOne({ + where: { username }, }); - if (!user) { - user = this.userRepo.create({ - username: input.login, - email: input.email, - displayName: input.displayName, - avatarUrl: input.avatarUrl, - roles: [UserRole.CONTRIBUTOR], + if (existingUsername) { + username = await this.generateUniqueUsername(input.login); + } + + let email: string | null = input.email; + if (email !== null) { + const existingEmail = await this.userRepo.findOne({ + where: { email }, }); - user = await this.userRepo.save(user); + if (existingEmail) { + email = null; + } } + let user = this.userRepo.create({ + username, + email, + displayName: input.displayName, + avatarUrl: input.avatarUrl, + roles: [UserRole.CONTRIBUTOR], + }); + user = await this.userRepo.save(user); + account = this.githubAccountRepo.create({ githubId: input.githubId, login: input.login, @@ -87,6 +146,20 @@ export class UsersService { return this.findOneRaw(user.id); } + private async generateUniqueUsername(base: string): Promise { + let candidate = base; + let counter = 0; + while (await this.userRepo.findOne({ where: { username: candidate } })) { + counter += 1; + candidate = `${base}-${counter}`; + if (counter > 100) { + candidate = `${base}-${Date.now()}-${counter}`; + break; + } + } + return candidate; + } + // Role assignment is intentionally out of scope for this version: a User's // `roles` are seeded to [UserRole.CONTRIBUTOR] at creation (upsertFromGithub) // and are not mutated by any API endpoint or admin flow today. The