fix: SSO-MFA-Umgehung, Datei-Magic-Bytes und oeffentliches Swagger geschlossen
Weitere vom Sicherheitsaudit gemeldete Luecken behoben, die bisher als niedrige Prioritaet offen gelassen wurden: - SSO-Login (Entra ID) prüft jetzt lokale MFA genauso wie der Passwort-Login: ist mfaEnabled gesetzt, liefert /auth/sso/complete denselben mfaRequired-Codepfad wie /auth/login, bevor Tokens ausgestellt werden. Frontend (Login-Seite, AuthContext) zeigt in dem Fall den bestehenden Code-Eingabe-Schritt statt die Anmeldung stillschweigend abzuschliessen. - Avatar- und Dokument-Uploads pruefen jetzt die tatsaechlichen Datei-Bytes (Magic Numbers fuer PNG/JPEG/WEBP/PDF) statt nur dem client-angegebenen MIME-Typ/data:-URL-Praefix zu vertrauen. - Swagger-UI (/api/v1/docs) ist jetzt standardmaessig deaktiviert (ENABLE_SWAGGER=true erforderlich), da sie zuvor das komplette API-Schema ungeschuetzt oeffentlich preisgab. npm-audit-Warnungen (deepmerge-ts/mysql2 unter prisma) bewusst nicht gefixt: kein stabiles Prisma-Release loest sie bislang auf, nur eine Release-Candidate-Hauptversion - das Risiko eines Downgrades/RC-Upgrades der Datenbankschicht uebersteigt den Nutzen fuer eine Kette, die zur Laufzeit ohnehin nicht erreichbar ist (nur Prisma-CLI, nur MySQL-Treiber, dieses Projekt nutzt Postgres). Co-Authored-By: Claude Sonnet 5 <[email protected]>
This commit is contained in:
1 parent
1b16b49913
commit
b7ff92abd3
16 files changed
+225
-28
No files matched your search
@@ -28,3 +28,8 @@ OIDC_ISSUER_URL="https://login.microsoftonline.com/<tenant-id>/v2.0"
|
||||
OIDC_CLIENT_ID="change-me-entra-client-id"
|
||||
OIDC_CLIENT_SECRET="change-me-entra-client-secret"
|
||||
OIDC_REDIRECT_URI="http://localhost:3001/api/v1/auth/sso/callback"
|
||||
|
||||
# Swagger-UI unter /api/v1/docs ist per Default deaktiviert, da sie das
|
||||
# gesamte API-Schema ungeschuetzt (kein JwtAuthGuard) preisgeben wuerde.
|
||||
# Nur fuer lokale Entwicklung/Debugging explizit aktivieren.
|
||||
ENABLE_SWAGGER=false
|
||||
@@ -25,7 +25,7 @@ import {
|
||||
setRefreshTokenCookie,
|
||||
} from './util/refresh-token-cookie';
|
||||
|
||||
interface MfaRequiredResponse {
|
||||
export interface MfaRequiredResponse {
|
||||
mfaRequired: true;
|
||||
mfaToken: string;
|
||||
}
|
||||
|
||||
@@ -240,6 +240,9 @@ describe('AuthService', () => {
|
||||
|
||||
const result = await authService.issueTokensForSsoUser(activeUser.id);
|
||||
|
||||
if (result.mfaRequired) {
|
||||
throw new Error('expected mfaRequired to be false');
|
||||
}
|
||||
expect(result.accessToken).toBe('signed.jwt.token');
|
||||
expect(prisma.refreshToken.create).toHaveBeenCalled();
|
||||
expect(auditLog.record).toHaveBeenCalledWith(
|
||||
@@ -247,6 +250,26 @@ describe('AuthService', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('returns an MFA challenge instead of tokens when the SSO user has local MFA enabled', async () => {
|
||||
usersService.findById.mockResolvedValue({
|
||||
...activeUser,
|
||||
mfaEnabled: true,
|
||||
mfaSecret: 'encrypted-secret',
|
||||
});
|
||||
|
||||
const result = await authService.issueTokensForSsoUser(activeUser.id);
|
||||
|
||||
if (!result.mfaRequired) {
|
||||
throw new Error('expected mfaRequired to be true');
|
||||
}
|
||||
expect(result.mfaToken).toBe('mfa-challenge-token');
|
||||
expect(mfaService.signChallengeToken).toHaveBeenCalledWith(activeUser.id);
|
||||
expect(prisma.refreshToken.create).not.toHaveBeenCalled();
|
||||
expect(auditLog.record).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ action: 'LOGIN' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('throws when the user is deactivated', async () => {
|
||||
usersService.findById.mockResolvedValue({
|
||||
...activeUser,
|
||||
|
||||
@@ -144,12 +144,24 @@ export class AuthService {
|
||||
return { mfaRequired: false, accessToken, refreshToken };
|
||||
}
|
||||
|
||||
/** Issues our own access/refresh token pair for a user already authenticated via SSO. */
|
||||
async issueTokensForSsoUser(userId: string): Promise<AuthTokens> {
|
||||
/**
|
||||
* Issues our own access/refresh token pair for a user already
|
||||
* authenticated via SSO. Local MFA is still enforced here, same as
|
||||
* password login: SSO establishes identity, not the second factor, so a
|
||||
* user who has explicitly enabled TOTP must complete it here too rather
|
||||
* than getting it silently skipped by signing in via Microsoft instead.
|
||||
*/
|
||||
async issueTokensForSsoUser(userId: string): Promise<LoginResult> {
|
||||
const user = await this.usersService.findById(userId);
|
||||
if (!user.isActive) {
|
||||
throw new UnauthorizedException('Invalid credentials');
|
||||
}
|
||||
if (user.mfaEnabled) {
|
||||
return {
|
||||
mfaRequired: true,
|
||||
mfaToken: this.mfaService.signChallengeToken(user.id),
|
||||
};
|
||||
}
|
||||
const accessToken = this.signAccessToken({
|
||||
id: user.id,
|
||||
email: user.email,
|
||||
@@ -161,7 +173,7 @@ export class AuthService {
|
||||
action: 'LOGIN',
|
||||
userId: user.id,
|
||||
});
|
||||
return { accessToken, refreshToken };
|
||||
return { mfaRequired: false, accessToken, refreshToken };
|
||||
}
|
||||
|
||||
async verifyMfaAndLogin(mfaToken: string, code: string): Promise<AuthTokens> {
|
||||
|
||||
@@ -13,6 +13,7 @@ import { Throttle } from '@nestjs/throttler';
|
||||
import type { Request, Response } from 'express';
|
||||
import { Public } from '../decorators/public.decorator';
|
||||
import { AuthService } from '../auth.service';
|
||||
import { MfaRequiredResponse } from '../auth.controller';
|
||||
import { AccessTokenDto } from '../dto/access-token.dto';
|
||||
import { SENSITIVE_ACTION_THROTTLE } from '../util/throttle';
|
||||
import { setRefreshTokenCookie } from '../util/refresh-token-cookie';
|
||||
@@ -121,7 +122,7 @@ export class SsoController {
|
||||
async complete(
|
||||
@Req() req: Request,
|
||||
@Res({ passthrough: true }) res: Response,
|
||||
): Promise<AccessTokenDto> {
|
||||
): Promise<AccessTokenDto | MfaRequiredResponse> {
|
||||
const ssoCode = (req.cookies as Record<string, string> | undefined)?.[
|
||||
SSO_CODE_COOKIE
|
||||
];
|
||||
@@ -132,10 +133,12 @@ export class SsoController {
|
||||
);
|
||||
}
|
||||
const { userId } = this.ssoService.verifySsoCode(ssoCode);
|
||||
const { accessToken, refreshToken } =
|
||||
await this.authService.issueTokensForSsoUser(userId);
|
||||
setRefreshTokenCookie(res, refreshToken);
|
||||
return { accessToken };
|
||||
const result = await this.authService.issueTokensForSsoUser(userId);
|
||||
if (result.mfaRequired) {
|
||||
return { mfaRequired: true, mfaToken: result.mfaToken };
|
||||
}
|
||||
setRefreshTokenCookie(res, result.refreshToken);
|
||||
return { accessToken: result.accessToken };
|
||||
}
|
||||
|
||||
private parsePkceCookie(raw: string | undefined): PkceCookiePayload | null {
|
||||
|
||||
@@ -0,0 +1,49 @@
|
||||
import { matchesFileSignature } from './file-signature';
|
||||
|
||||
describe('matchesFileSignature', () => {
|
||||
it('accepts a buffer starting with the PNG signature', () => {
|
||||
const png = Buffer.from([
|
||||
0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00,
|
||||
]);
|
||||
expect(matchesFileSignature(png, 'image/png')).toBe(true);
|
||||
});
|
||||
|
||||
it('accepts a buffer starting with the JPEG signature', () => {
|
||||
const jpeg = Buffer.from([0xff, 0xd8, 0xff, 0x00]);
|
||||
expect(matchesFileSignature(jpeg, 'image/jpeg')).toBe(true);
|
||||
});
|
||||
|
||||
it('accepts a RIFF/WEBP container', () => {
|
||||
const webp = Buffer.concat([
|
||||
Buffer.from('RIFF', 'ascii'),
|
||||
Buffer.from([0, 0, 0, 0]),
|
||||
Buffer.from('WEBP', 'ascii'),
|
||||
]);
|
||||
expect(matchesFileSignature(webp, 'image/webp')).toBe(true);
|
||||
});
|
||||
|
||||
it('accepts a PDF header', () => {
|
||||
const pdf = Buffer.from('%PDF-1.7\n', 'ascii');
|
||||
expect(matchesFileSignature(pdf, 'application/pdf')).toBe(true);
|
||||
});
|
||||
|
||||
it('rejects a buffer whose bytes do not match the declared type', () => {
|
||||
const plainText = Buffer.from('this is not an image', 'ascii');
|
||||
expect(matchesFileSignature(plainText, 'image/png')).toBe(false);
|
||||
expect(matchesFileSignature(plainText, 'image/jpeg')).toBe(false);
|
||||
expect(matchesFileSignature(plainText, 'image/webp')).toBe(false);
|
||||
expect(matchesFileSignature(plainText, 'application/pdf')).toBe(false);
|
||||
});
|
||||
|
||||
it('rejects a buffer shorter than the signature it claims to have', () => {
|
||||
expect(matchesFileSignature(Buffer.from([0xff, 0xd8]), 'image/jpeg')).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it('allows any bytes through for a mime type with no known signature', () => {
|
||||
expect(matchesFileSignature(Buffer.from('anything'), 'text/plain')).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,35 @@
|
||||
const SIGNATURE_CHECKS: Record<string, (buffer: Buffer) => boolean> = {
|
||||
'image/png': (buffer) =>
|
||||
buffer.length >= 8 &&
|
||||
buffer
|
||||
.subarray(0, 8)
|
||||
.equals(Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a])),
|
||||
'image/jpeg': (buffer) =>
|
||||
buffer.length >= 3 &&
|
||||
buffer[0] === 0xff &&
|
||||
buffer[1] === 0xd8 &&
|
||||
buffer[2] === 0xff,
|
||||
'image/webp': (buffer) =>
|
||||
buffer.length >= 12 &&
|
||||
buffer.subarray(0, 4).toString('ascii') === 'RIFF' &&
|
||||
buffer.subarray(8, 12).toString('ascii') === 'WEBP',
|
||||
'application/pdf': (buffer) =>
|
||||
buffer.length >= 5 && buffer.subarray(0, 5).toString('ascii') === '%PDF-',
|
||||
};
|
||||
|
||||
/**
|
||||
* Verifies a file's actual bytes match its declared MIME type instead of
|
||||
* trusting the client-supplied value alone (a multipart Content-Type header
|
||||
* or a data: URL prefix are both attacker-controlled). Used at every upload
|
||||
* path that persists the bytes to disk under a server-chosen name/extension,
|
||||
* so a mislabeled file can't be dropped onto the filesystem as a type it
|
||||
* isn't. Types with no signature defined here are not checked (returns
|
||||
* true) - callers still gate on their own MIME allowlist first.
|
||||
*/
|
||||
export function matchesFileSignature(
|
||||
buffer: Buffer,
|
||||
declaredMimeType: string,
|
||||
): boolean {
|
||||
const check = SIGNATURE_CHECKS[declaredMimeType];
|
||||
return check ? check(buffer) : true;
|
||||
}
|
||||
@@ -79,7 +79,7 @@ describe('DocumentsService', () => {
|
||||
|
||||
describe('create', () => {
|
||||
const file = {
|
||||
buffer: Buffer.from('binary'),
|
||||
buffer: Buffer.from([0xff, 0xd8, 0xff, 0x00, 0x01, 0x02]),
|
||||
originalname: 'bild.jpg',
|
||||
mimetype: 'image/jpeg',
|
||||
size: 1234,
|
||||
@@ -95,6 +95,14 @@ describe('DocumentsService', () => {
|
||||
).rejects.toBeInstanceOf(BadRequestException);
|
||||
});
|
||||
|
||||
it('rejects a file whose bytes do not match the declared mime type', async () => {
|
||||
const mislabeled = { ...file, buffer: Buffer.from('not-a-jpeg') };
|
||||
await expect(
|
||||
service.create(mislabeled, 'COLONY', 'colony-1', 'user-1'),
|
||||
).rejects.toBeInstanceOf(BadRequestException);
|
||||
expect(fs.writeFile).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('throws NotFoundException when the referenced entity does not exist', async () => {
|
||||
prisma.colony.findUnique.mockResolvedValue(null);
|
||||
await expect(
|
||||
|
||||
@@ -8,6 +8,7 @@ import * as fs from 'node:fs/promises';
|
||||
import * as path from 'node:path';
|
||||
import { PrismaService } from '../prisma/prisma.service';
|
||||
import { AuditLogService } from '../audit/audit-log.service';
|
||||
import { matchesFileSignature } from '../common/file-signature';
|
||||
import { Document, DocumentEntityType } from '../../generated/prisma';
|
||||
import { ALLOWED_DOCUMENT_MIME_TYPES } from './document-mime-types';
|
||||
|
||||
@@ -86,6 +87,11 @@ export class DocumentsService {
|
||||
if (!(file.mimetype in ALLOWED_DOCUMENT_MIME_TYPES)) {
|
||||
throw new BadRequestException('Nicht unterstützter Dateityp');
|
||||
}
|
||||
if (!matchesFileSignature(file.buffer, file.mimetype)) {
|
||||
throw new BadRequestException(
|
||||
'Datei entspricht nicht dem angegebenen Format',
|
||||
);
|
||||
}
|
||||
await this.assertEntityExists(entityType, entityId);
|
||||
|
||||
const extension = ALLOWED_DOCUMENT_MIME_TYPES[file.mimetype];
|
||||
|
||||
@@ -23,14 +23,21 @@ async function bootstrap() {
|
||||
|
||||
configureApp(app);
|
||||
|
||||
const swaggerConfig = new DocumentBuilder()
|
||||
.setTitle('ApiCore API')
|
||||
.setDescription('Digitale Imkereiverwaltung – API-Dokumentation')
|
||||
.setVersion('1.0')
|
||||
.addBearerAuth()
|
||||
.build();
|
||||
const swaggerDocument = SwaggerModule.createDocument(app, swaggerConfig);
|
||||
SwaggerModule.setup('api/v1/docs', app, swaggerDocument);
|
||||
// Swagger is mounted as raw Express middleware outside Nest's routing, so
|
||||
// it is never covered by the global JwtAuthGuard - anyone reaching the
|
||||
// host could otherwise read the full API schema (every endpoint and DTO
|
||||
// shape) with no authentication at all. Off by default; an operator who
|
||||
// wants it in a given environment opts in explicitly.
|
||||
if (process.env.ENABLE_SWAGGER === 'true') {
|
||||
const swaggerConfig = new DocumentBuilder()
|
||||
.setTitle('ApiCore API')
|
||||
.setDescription('Digitale Imkereiverwaltung – API-Dokumentation')
|
||||
.setVersion('1.0')
|
||||
.addBearerAuth()
|
||||
.build();
|
||||
const swaggerDocument = SwaggerModule.createDocument(app, swaggerConfig);
|
||||
SwaggerModule.setup('api/v1/docs', app, swaggerDocument);
|
||||
}
|
||||
|
||||
await app.listen(process.env.PORT ?? 3001);
|
||||
}
|
||||
|
||||
@@ -178,7 +178,8 @@ describe('ProfileService', () => {
|
||||
});
|
||||
|
||||
describe('uploadAvatar', () => {
|
||||
const dataUrl = `data:image/jpeg;base64,${Buffer.from('binary').toString('base64')}`;
|
||||
const jpegBytes = Buffer.from([0xff, 0xd8, 0xff, 0x00, 0x01, 0x02]);
|
||||
const dataUrl = `data:image/jpeg;base64,${jpegBytes.toString('base64')}`;
|
||||
|
||||
it('rejects a value that is not a data: URL', async () => {
|
||||
await expect(
|
||||
@@ -200,6 +201,14 @@ describe('ProfileService', () => {
|
||||
).rejects.toBeInstanceOf(BadRequestException);
|
||||
});
|
||||
|
||||
it('rejects bytes that do not match the declared mime type', async () => {
|
||||
const mislabeled = `data:image/jpeg;base64,${Buffer.from('not-a-jpeg').toString('base64')}`;
|
||||
await expect(
|
||||
service.uploadAvatar('user-1', mislabeled),
|
||||
).rejects.toBeInstanceOf(BadRequestException);
|
||||
expect(fs.writeFile).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('writes the file, updates avatarPath, and deletes the old file if present', async () => {
|
||||
prisma.user.findUnique.mockResolvedValue({
|
||||
...baseUser,
|
||||
|
||||
@@ -11,6 +11,7 @@ import * as fs from 'node:fs/promises';
|
||||
import * as path from 'node:path';
|
||||
import { PrismaService } from '../prisma/prisma.service';
|
||||
import { AuditLogService } from '../audit/audit-log.service';
|
||||
import { matchesFileSignature } from '../common/file-signature';
|
||||
import { MfaService, MfaEnrollment } from '../auth/mfa/mfa.service';
|
||||
import { PermissionResolverService } from '../auth/permissions/permission-resolver.service';
|
||||
import { UsersService } from '../users/users.service';
|
||||
@@ -155,6 +156,11 @@ export class ProfileService {
|
||||
if (buffer.length > MAX_AVATAR_SIZE_BYTES) {
|
||||
throw new BadRequestException('Datei zu groß (max. 2 MB)');
|
||||
}
|
||||
if (!matchesFileSignature(buffer, mimeType)) {
|
||||
throw new BadRequestException(
|
||||
'Datei entspricht nicht dem angegebenen Format',
|
||||
);
|
||||
}
|
||||
const user = await this.requireUser(userId);
|
||||
const extension = ALLOWED_AVATAR_MIME_TYPES[mimeType];
|
||||
const directory = avatarStorageRoot();
|
||||
|
||||
Reference in new issue
Block a user