diff --git a/SECURITY_FIXES_AUTH.md b/SECURITY_FIXES_AUTH.md new file mode 100644 index 00000000..2cb3e433 --- /dev/null +++ b/SECURITY_FIXES_AUTH.md @@ -0,0 +1,43 @@ +# Auth security fixes + +Branch: `fix/auth-jti-suspended-apikey-lookup` + +## 1. Missing `jti` in signed JWTs (bug) +`AuthService.signToken()` now includes a unique `jti` (`crypto.randomUUID()`) +in every issued token. `JwtStrategy.validate()` already fell back to +`payload.jti ?? payload.sub` for the blacklist/session cache key — without a +`jti`, every token for a given merchant shared the same key, so revoking one +session would have blacklisted all of that merchant's sessions/devices at +once. With a per-token `jti`, blacklist/session-cache keys are now per-token. + +## 2. Suspended merchants could still log in +`AuthService.login()` verified the merchant exists and the password matches, +but never checked `merchant.status`. A suspended merchant (e.g. flagged for +fraud/AML) with correct credentials could still obtain a valid access token. +`login()` now throws `UnauthorizedException('Account suspended')` when +`status === MerchantStatus.SUSPENDED`. + +## 3. Suspended status wasn't re-checked for existing tokens +Even after fixing (2), a merchant suspended *after* issuing a token could +keep using that token for its full lifetime. `JwtStrategy.validate()` now +re-checks `merchant.status` in its DB-fallback path and rejects suspended +accounts. + +## 4. Linear bcrypt scan on every API-key request (perf / DoS) +`AuthService.findMerchantByApiKey()` loaded every merchant with a non-null +`apiKeyHash` and ran `bcrypt.compare` in a loop until it found a match. This +is invoked on every request authenticated via `X-API-Key` +(`JwtAuthGuard.canActivate()`), making latency scale linearly with merchant +count and giving any client an easy CPU-exhaustion lever. + +Added an indexed `apiKeyLookupHash` column (SHA-256 hex digest of the raw +key) on `Merchant`. `MerchantsService.generateApiKey()` now populates it +alongside the existing bcrypt hash. `findMerchantByApiKey()` looks up the +single candidate row by `apiKeyLookupHash` (O(1), indexed, unique) and only +then runs one `bcrypt.compare` against that candidate. + +Migration: `src/database/migrations/1772300000002-AddMerchantApiKeyLookupHash.ts`. + +**Note:** merchants with an API key issued before this migration will need +to regenerate it — the raw key needed to compute the new lookup hash can't +be recovered from the existing bcrypt hash. diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index a70cfaaf..1f1c0eaa 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -1,8 +1,9 @@ import { Injectable, UnauthorizedException, ConflictException } from '@nestjs/common'; import { JwtService } from '@nestjs/jwt'; import { InjectRepository } from '@nestjs/typeorm'; -import { Repository, Not, IsNull } from 'typeorm'; +import { Repository } from 'typeorm'; import * as bcrypt from 'bcrypt'; +import { randomUUID, createHash } from 'crypto'; import { Merchant, MerchantStatus } from '../merchants/entities/merchant.entity'; import { RegisterDto } from './dto/register.dto'; import { LoginDto } from './dto/login.dto'; @@ -46,6 +47,10 @@ export class AuthService { const valid = await bcrypt.compare(dto.password, merchant.passwordHash); if (!valid) throw new UnauthorizedException('Invalid credentials'); + if (merchant.status === MerchantStatus.SUSPENDED) { + throw new UnauthorizedException('Account suspended'); + } + const token = this.signToken(merchant.id, merchant.email, merchant.role); return { accessToken: token, merchant }; } @@ -60,18 +65,16 @@ export class AuthService { } async findMerchantByApiKey(rawKey: string): Promise { - const merchants = await this.merchantsRepo.find({ - where: { apiKeyHash: Not(IsNull()) }, + const lookupHash = createHash('sha256').update(rawKey).digest('hex'); + const merchant = await this.merchantsRepo.findOne({ + where: { apiKeyLookupHash: lookupHash }, }); - for (const m of merchants) { - if (m.apiKeyHash && (await bcrypt.compare(rawKey, m.apiKeyHash))) { - return m; - } - } - return null; + + if (!merchant?.apiKeyHash) return null; + return (await bcrypt.compare(rawKey, merchant.apiKeyHash)) ? merchant : null; } private signToken(sub: string, email: string, role?: string): string { - return this.jwtService.sign({ sub, email, role }); + return this.jwtService.sign({ sub, email, role, jti: randomUUID() }); } } diff --git a/src/auth/strategies/jwt.strategy.ts b/src/auth/strategies/jwt.strategy.ts index 494c8d81..6a877984 100644 --- a/src/auth/strategies/jwt.strategy.ts +++ b/src/auth/strategies/jwt.strategy.ts @@ -4,7 +4,7 @@ import { ExtractJwt, Strategy } from 'passport-jwt'; import { ConfigService } from '@nestjs/config'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; -import { Merchant } from '../../merchants/entities/merchant.entity'; +import { Merchant, MerchantStatus } from '../../merchants/entities/merchant.entity'; import { CacheService } from '../../cache/cache.service'; import { AuthService } from '../auth.service'; @@ -44,6 +44,9 @@ export class JwtStrategy extends PassportStrategy(Strategy) { // DB fallback const merchant = await this.merchantsRepo.findOne({ where: { id: payload.sub } }); if (!merchant) throw new UnauthorizedException('Merchant not found'); + if (merchant.status === MerchantStatus.SUSPENDED) { + throw new UnauthorizedException('Account suspended'); + } const result = { merchantId: merchant.id, email: merchant.email, role: merchant.role }; diff --git a/src/database/migrations/1772300000002-AddMerchantApiKeyLookupHash.ts b/src/database/migrations/1772300000002-AddMerchantApiKeyLookupHash.ts new file mode 100644 index 00000000..a915c335 --- /dev/null +++ b/src/database/migrations/1772300000002-AddMerchantApiKeyLookupHash.ts @@ -0,0 +1,25 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class AddMerchantApiKeyLookupHash1772300000002 implements MigrationInterface { + name = 'AddMerchantApiKeyLookupHash1772300000002'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query( + `ALTER TABLE "merchants" ADD COLUMN "api_key_lookup_hash" character varying`, + ); + await queryRunner.query( + `ALTER TABLE "merchants" ADD CONSTRAINT "UQ_merchants_api_key_lookup_hash" UNIQUE ("api_key_lookup_hash")`, + ); + await queryRunner.query( + `CREATE INDEX "IDX_merchants_api_key_lookup_hash" ON "merchants" ("api_key_lookup_hash")`, + ); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(`DROP INDEX "IDX_merchants_api_key_lookup_hash"`); + await queryRunner.query( + `ALTER TABLE "merchants" DROP CONSTRAINT "UQ_merchants_api_key_lookup_hash"`, + ); + await queryRunner.query(`ALTER TABLE "merchants" DROP COLUMN "api_key_lookup_hash"`); + } +} diff --git a/src/merchants/entities/merchant.entity.ts b/src/merchants/entities/merchant.entity.ts index b317b020..cffe952b 100644 --- a/src/merchants/entities/merchant.entity.ts +++ b/src/merchants/entities/merchant.entity.ts @@ -2,6 +2,7 @@ import { Entity, PrimaryGeneratedColumn, Column, + Index, CreateDateColumn, UpdateDateColumn, DeleteDateColumn, diff --git a/src/merchants/merchants.service.ts b/src/merchants/merchants.service.ts index d5436a56..b0f3c303 100644 --- a/src/merchants/merchants.service.ts +++ b/src/merchants/merchants.service.ts @@ -107,6 +107,7 @@ export class MerchantsService { merchant.apiKey = rawKey.substring(0, 12) + '...'; merchant.apiKeyHash = hash; + merchant.apiKeyLookupHash = crypto.createHash('sha256').update(rawKey).digest('hex'); merchant.apiKeyScopes = scopes?.length ? scopes : API_KEY_SCOPES; await this.merchantsRepo.save(merchant);