From 1b16b499130d484a7ca077197e469032fc05de1f Mon Sep 17 00:00:00 2001 From: Nick Szaunig Date: Wed, 2 Sep 2026 08:57:42 +0200 Subject: [PATCH] fix: Rechteausweitung ueber Rollenzuweisung und weitere Auth-Haertung Ein Sicherheitsaudit deckte auf, dass jede Rolle mit users:update sich selbst per PATCH /users/:id auf eine Admin-Rolle heraufstufen konnte, da roleId ohne eigene Berechtigung und ohne Selbstbezug-Pruefung akzeptiert wurde. Ausserdem lag ein fest im Code hinterlegtes Standard-Admin-Passwort vor, das bei fehlender SEED_ADMIN_PASSWORD-Env greift. - Neue Berechtigung users:assign-role, getrennt von users:create/update, fuer jede Rollenzuweisung erforderlich - Selbstaenderung der eigenen Rolle serverseitig blockiert (auch fuer Admins), ebenso Selbst-Reset der eigenen MFA ueber die Admin-Route - Seed erzeugt bei fehlender Env ein zufaelliges statt eines fixen Passworts - Wiederverwendung eines bereits rotierten Refresh-Tokens (Diebstahl- Indiz) widerruft nun alle Sessions des betroffenen Nutzers - Rate-Limit auf MFA-Enrollment-Bestaetigung ergaenzt Co-Authored-By: Claude Sonnet 5 --- ApiCore-Backend/prisma/seed.ts | 12 +++- ApiCore-Backend/src/auth/auth.service.spec.ts | 18 +++++- ApiCore-Backend/src/auth/auth.service.ts | 20 ++++++- .../auth/permissions/permission-catalog.ts | 1 + .../src/profile/profile.controller.ts | 1 + .../src/users/users.service.spec.ts | 57 ++++++++++++++++++- ApiCore-Backend/src/users/users.service.ts | 44 +++++++++++++- 7 files changed, 146 insertions(+), 7 deletions(-) diff --git a/ApiCore-Backend/prisma/seed.ts b/ApiCore-Backend/prisma/seed.ts index 66b026d..d2e06df 100644 --- a/ApiCore-Backend/prisma/seed.ts +++ b/ApiCore-Backend/prisma/seed.ts @@ -1,11 +1,19 @@ import 'dotenv/config'; import * as bcrypt from 'bcryptjs'; +import * as crypto from 'node:crypto'; import { PrismaPg } from '@prisma/adapter-pg'; import { Prisma, PrismaClient } from '../generated/prisma'; const BCRYPT_SALT_ROUNDS = 12; const DEFAULT_ADMIN_EMAIL = 'admin@apicore.local'; -const DEFAULT_ADMIN_PASSWORD = 'ChangeMe123!'; + +// No fixed fallback password: a fixed literal shipped in source would be a +// well-known, guessable credential for any deployment that forgets to set +// SEED_ADMIN_PASSWORD. A random one is generated instead and printed once +// below, same as an operator-supplied one would be. +function generateRandomPassword(): string { + return crypto.randomBytes(18).toString('base64url'); +} // Only printed the moment the account is actually created (first deploy on // an empty database), never on later restarts: by then the real admin may @@ -28,7 +36,7 @@ async function main(): Promise { }); const email = process.env.SEED_ADMIN_EMAIL ?? DEFAULT_ADMIN_EMAIL; - const password = process.env.SEED_ADMIN_PASSWORD ?? DEFAULT_ADMIN_PASSWORD; + const password = process.env.SEED_ADMIN_PASSWORD ?? generateRandomPassword(); const existing = await prisma.user.findUnique({ where: { email } }); if (existing) { console.log(`Seed: admin user ${email} already exists, skipping.`); diff --git a/ApiCore-Backend/src/auth/auth.service.spec.ts b/ApiCore-Backend/src/auth/auth.service.spec.ts index 3bdc59d..a128d07 100644 --- a/ApiCore-Backend/src/auth/auth.service.spec.ts +++ b/ApiCore-Backend/src/auth/auth.service.spec.ts @@ -19,7 +19,10 @@ describe('AuthService', () => { }; let jwtService: { sign: jest.Mock }; let auditLog: { record: jest.Mock }; - let permissionResolver: { getPermissions: jest.Mock }; + let permissionResolver: { + getPermissions: jest.Mock; + invalidateUser: jest.Mock; + }; let mfaService: { signChallengeToken: jest.Mock; verifyChallengeToken: jest.Mock; @@ -52,6 +55,7 @@ describe('AuthService', () => { auditLog = { record: jest.fn().mockResolvedValue(undefined) }; permissionResolver = { getPermissions: jest.fn().mockResolvedValue(new Set()), + invalidateUser: jest.fn(), }; mfaService = { signChallengeToken: jest.fn().mockReturnValue('mfa-challenge-token'), @@ -264,16 +268,26 @@ describe('AuthService', () => { ); }); - it('throws when the refresh token is revoked', async () => { + it('throws when the refresh token is revoked and revokes all other sessions for that user (reuse detection)', async () => { prisma.refreshToken.findUnique.mockResolvedValue({ id: 'rt-1', userId: activeUser.id, revokedAt: new Date(), expiresAt: new Date(Date.now() + 1000), }); + prisma.refreshToken.updateMany.mockResolvedValue({ count: 2 }); + await expect(authService.refresh('revoked-token')).rejects.toBeInstanceOf( UnauthorizedException, ); + expect(prisma.refreshToken.updateMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: { userId: activeUser.id, revokedAt: null }, + }), + ); + expect(permissionResolver.invalidateUser).toHaveBeenCalledWith( + activeUser.id, + ); }); it('throws when the refresh token is expired', async () => { diff --git a/ApiCore-Backend/src/auth/auth.service.ts b/ApiCore-Backend/src/auth/auth.service.ts index 3e2a6ad..066a317 100644 --- a/ApiCore-Backend/src/auth/auth.service.ts +++ b/ApiCore-Backend/src/auth/auth.service.ts @@ -207,7 +207,25 @@ export class AuthService { const stored = await this.prisma.refreshToken.findUnique({ where: { tokenHash }, }); - if (!stored || stored.revokedAt || stored.expiresAt < new Date()) { + if (!stored) { + throw new UnauthorizedException('Invalid refresh token'); + } + if (stored.revokedAt) { + // Reuse of an already-rotated-out refresh token is a strong signal + // it was stolen (a legitimate client only ever presents the newest + // token) - revoke every other active session for this user rather + // than trusting rotation alone to have contained the leak. + await this.prisma.refreshToken.updateMany({ + where: { userId: stored.userId, revokedAt: null }, + data: { revokedAt: new Date() }, + }); + this.permissionResolver.invalidateUser(stored.userId); + this.logger.warn( + `Refresh token reuse detected for user ${stored.userId}; all sessions revoked`, + ); + throw new UnauthorizedException('Invalid refresh token'); + } + if (stored.expiresAt < new Date()) { throw new UnauthorizedException('Invalid refresh token'); } const user = await this.usersService.findById(stored.userId); diff --git a/ApiCore-Backend/src/auth/permissions/permission-catalog.ts b/ApiCore-Backend/src/auth/permissions/permission-catalog.ts index d4685a7..33a858e 100644 --- a/ApiCore-Backend/src/auth/permissions/permission-catalog.ts +++ b/ApiCore-Backend/src/auth/permissions/permission-catalog.ts @@ -255,6 +255,7 @@ export const PERMISSION_CATALOG = [ ['create', 'Anlegen'], ['update', 'Bearbeiten'], ['deactivate', 'Deaktivieren'], + ['assign-role', 'Rolle zuweisen'], ], 2300, ), diff --git a/ApiCore-Backend/src/profile/profile.controller.ts b/ApiCore-Backend/src/profile/profile.controller.ts index d8d1382..5511173 100644 --- a/ApiCore-Backend/src/profile/profile.controller.ts +++ b/ApiCore-Backend/src/profile/profile.controller.ts @@ -100,6 +100,7 @@ export class ProfileController { return this.profileService.enrollMfa(actor.id, actor.email); } + @Throttle(SENSITIVE_ACTION_THROTTLE) @Post('mfa/enroll/confirm') @HttpCode(204) confirmMfaEnrollment( diff --git a/ApiCore-Backend/src/users/users.service.spec.ts b/ApiCore-Backend/src/users/users.service.spec.ts index 93fbb3a..841bf7f 100644 --- a/ApiCore-Backend/src/users/users.service.spec.ts +++ b/ApiCore-Backend/src/users/users.service.spec.ts @@ -1,4 +1,8 @@ -import { ConflictException, NotFoundException } from '@nestjs/common'; +import { + ConflictException, + ForbiddenException, + NotFoundException, +} from '@nestjs/common'; import { UsersService } from './users.service'; import { PrismaService } from '../prisma/prisma.service'; import { AuditLogService } from '../audit/audit-log.service'; @@ -22,6 +26,7 @@ describe('UsersService', () => { let permissionResolver: { invalidateUser: jest.Mock; invalidateAll: jest.Mock; + hasAny: jest.Mock; }; const staffRole = { @@ -77,6 +82,7 @@ describe('UsersService', () => { permissionResolver = { invalidateUser: jest.fn(), invalidateAll: jest.fn(), + hasAny: jest.fn().mockResolvedValue(true), }; usersService = new UsersService( prisma as unknown as PrismaService, @@ -324,4 +330,53 @@ describe('UsersService', () => { ).rejects.toBeInstanceOf(ConflictException); expect(prisma.user.update).not.toHaveBeenCalled(); }); + + it('update throws ForbiddenException when the caller tries to change their own role', async () => { + prisma.user.findUnique.mockResolvedValue(adminUser); + + await expect( + usersService.update(adminUser.id, { roleId: staffRole.id }, adminUser.id), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(prisma.user.update).not.toHaveBeenCalled(); + }); + + it('update throws ForbiddenException when the caller lacks users:assign-role', async () => { + prisma.user.findUnique.mockResolvedValue(existingUser); + prisma.role.findUnique.mockResolvedValue(adminRole); + permissionResolver.hasAny.mockResolvedValue(false); + + await expect( + usersService.update( + existingUser.id, + { roleId: adminRole.id }, + adminUser.id, + ), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(prisma.user.update).not.toHaveBeenCalled(); + }); + + it('create throws ForbiddenException when the caller lacks users:assign-role', async () => { + prisma.user.findUnique.mockResolvedValue(null); + permissionResolver.hasAny.mockResolvedValue(false); + + await expect( + usersService.create( + { + email: 'new@example.com', + password: 'password123', + name: 'New User', + roleId: staffRole.id, + }, + adminUser.id, + ), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(prisma.user.create).not.toHaveBeenCalled(); + }); + + it('resetMfa throws ForbiddenException when targeting your own account', async () => { + await expect( + usersService.resetMfa(adminUser.id, adminUser.id), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(prisma.user.update).not.toHaveBeenCalled(); + }); }); diff --git a/ApiCore-Backend/src/users/users.service.ts b/ApiCore-Backend/src/users/users.service.ts index 0d97789..c0f3665 100644 --- a/ApiCore-Backend/src/users/users.service.ts +++ b/ApiCore-Backend/src/users/users.service.ts @@ -1,5 +1,6 @@ import { ConflictException, + ForbiddenException, Injectable, NotFoundException, } from '@nestjs/common'; @@ -55,6 +56,27 @@ export class UsersService { }; } + /** + * Assigning a role is gated by its own permission, separate from + * users:create/users:update - otherwise any role holding those (e.g. a + * minimal "user manager" role) could hand out any role, including a + * system/admin one, to itself or a colluding account. actingUserId is + * only absent for non-HTTP callers (there are none today, and tests that + * exercise the service directly without an actor), where there is no + * caller to check permissions against. + */ + private async assertCanAssignRole(actingUserId?: string): Promise { + if (!actingUserId) { + return; + } + const allowed = await this.permissionResolver.hasAny(actingUserId, [ + 'users:assign-role', + ]); + if (!allowed) { + throw new ForbiddenException('Missing permission to assign roles'); + } + } + private async assertActiveRole(roleId: string): Promise { const role = await this.prisma.role.findUnique({ where: { id: roleId } }); if (!role) { @@ -127,6 +149,7 @@ export class UsersService { throw new ConflictException('Email already in use'); } await this.assertActiveRole(dto.roleId); + await this.assertCanAssignRole(actingUserId); const passwordHash = await bcrypt.hash(dto.password, BCRYPT_SALT_ROUNDS); const user = await this.prisma.user.create({ data: { @@ -158,6 +181,13 @@ export class UsersService { await this.assertActiveRole(dto.roleId); } if (roleChanged) { + // Role changes for yourself always require a second party, regardless + // of what permission the caller holds - otherwise anyone who can + // reach this endpoint at all could grant themselves admin. + if (actingUserId && id === actingUserId) { + throw new ForbiddenException('Cannot change your own role'); + } + await this.assertCanAssignRole(actingUserId); await this.assertNotLastSystemRoleUser(id); } const user = await this.prisma.user.update({ @@ -202,8 +232,20 @@ export class UsersService { return this.toSummary(user); } - /** Recovery path for a user who lost their MFA device (no email-reset flow exists in this project). */ + /** + * Recovery path for a user who lost their MFA device (no email-reset flow + * exists in this project). Deliberately blocked for the caller's own + * account: unlike resetting someone else's MFA, self-service MFA removal + * belongs behind a password check (POST /profile/mfa/disable) - otherwise + * a stolen access token alone (no password) would be enough to strip the + * victim's own MFA protection via this admin route. + */ async resetMfa(id: string, actingUserId?: string): Promise { + if (actingUserId && id === actingUserId) { + throw new ForbiddenException( + 'Use /profile/mfa/disable to disable your own MFA', + ); + } await this.findById(id); const user = await this.prisma.user.update({ where: { id },