From 4bd7e9791a71125b15a64a8c883b3da3736cf055 Mon Sep 17 00:00:00 2001 From: Nick Szaunig Date: Wed, 2 Sep 2026 15:13:02 +0200 Subject: [PATCH] fix: Robustheits-Luecken aus zweitem, unabhaengigem Sicherheitsaudit behoben Ein zweiter, unabhaengiger Audit-Durchlauf (frischer Blick, keine Vorkenntnis meiner eigenen Fixes) bestaetigte alle bisherigen Fixes als korrekt verdrahtet und fand 3 LOW-severity Robustheitsluecken - keine davon war ausnutzbar, aber alle sind jetzt behoben: - HarvestsService.create/ExtractionsService.create pruefen jetzt, dass referenzierte colonyIds/harvestIds tatsaechlich existieren, bevor die Transaktion laeuft - vorher fuehrte eine unbekannte Id zu einem ungefangenen 500 mit rohem Prisma-Fehler statt einer sauberen 400. - Etiketten-Bildelemente durchlaufen jetzt denselben Magic-Byte-Check wie Avatar-/Dokument-Uploads, statt pdfkits Bildparser ungepruefte Bytes zu uebergeben. - Webhook-Secret-Redaktion im Audit-Log nutzt jetzt explizites delete statt sich auf Prismas JSON-Serialisierung zu verlassen, die undefined-Werte verwirft (fragil, falls sich die Serialisierung mal aendert). Co-Authored-By: Claude Sonnet 5 --- .../extractions/extractions.service.spec.ts | 20 +++++- .../src/extractions/extractions.service.ts | 21 +++++- .../src/harvests/harvests.service.spec.ts | 24 ++++++- .../src/harvests/harvests.service.ts | 21 +++++- .../label-templates.service.spec.ts | 69 +++++++++++++++++++ .../label-templates.service.ts | 9 ++- .../src/webhooks/webhooks.service.ts | 16 ++++- 7 files changed, 173 insertions(+), 7 deletions(-) diff --git a/ApiCore-Backend/src/extractions/extractions.service.spec.ts b/ApiCore-Backend/src/extractions/extractions.service.spec.ts index 0b12b48..a1c296f 100644 --- a/ApiCore-Backend/src/extractions/extractions.service.spec.ts +++ b/ApiCore-Backend/src/extractions/extractions.service.spec.ts @@ -1,4 +1,4 @@ -import { NotFoundException } from '@nestjs/common'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; import { ExtractionsService } from './extractions.service'; import { PrismaService } from '../prisma/prisma.service'; import { AuditLogService } from '../audit/audit-log.service'; @@ -7,6 +7,7 @@ describe('ExtractionsService', () => { let service: ExtractionsService; let prisma: { extraction: { findUnique: jest.Mock; update: jest.Mock }; + harvest: { findMany: jest.Mock }; $transaction: jest.Mock; }; let auditLog: { record: jest.Mock }; @@ -24,6 +25,11 @@ describe('ExtractionsService', () => { }; prisma = { extraction: { findUnique: jest.fn(), update: jest.fn() }, + harvest: { + findMany: jest + .fn() + .mockResolvedValue([{ id: 'harvest-1' }, { id: 'harvest-2' }]), + }, $transaction: jest.fn((callback: (txArg: typeof tx) => unknown) => callback(tx), ), @@ -59,6 +65,18 @@ describe('ExtractionsService', () => { ]); }); + it('create rejects an unknown harvest id instead of failing on the DB foreign-key constraint', async () => { + prisma.harvest.findMany.mockResolvedValue([{ id: 'harvest-1' }]); + + await expect( + service.create( + { date: '2026-07-05', harvestIds: ['harvest-1', 'missing-harvest'] }, + 'user-1', + ), + ).rejects.toBeInstanceOf(BadRequestException); + expect(prisma.$transaction).not.toHaveBeenCalled(); + }); + it('update writes an audit log entry with before and after data', async () => { const updated = { ...extraction, notes: 'geändert' }; prisma.extraction.findUnique diff --git a/ApiCore-Backend/src/extractions/extractions.service.ts b/ApiCore-Backend/src/extractions/extractions.service.ts index 75d509e..462855c 100644 --- a/ApiCore-Backend/src/extractions/extractions.service.ts +++ b/ApiCore-Backend/src/extractions/extractions.service.ts @@ -1,4 +1,8 @@ -import { Injectable, NotFoundException } from '@nestjs/common'; +import { + BadRequestException, + Injectable, + NotFoundException, +} from '@nestjs/common'; import { PrismaService } from '../prisma/prisma.service'; import { AuditLogService } from '../audit/audit-log.service'; import { CreateExtractionDto } from './dto/create-extraction.dto'; @@ -37,7 +41,22 @@ export class ExtractionsService { return extraction; } + private async assertHarvestsExist(harvestIds: string[]): Promise { + const found = await this.prisma.harvest.findMany({ + where: { id: { in: harvestIds } }, + select: { id: true }, + }); + const foundIds = new Set(found.map((harvest) => harvest.id)); + const missing = harvestIds.filter((id) => !foundIds.has(id)); + if (missing.length > 0) { + throw new BadRequestException( + `Unknown harvest id(s): ${missing.join(', ')}`, + ); + } + } + async create(dto: CreateExtractionDto, actingUserId: string) { + await this.assertHarvestsExist(dto.harvestIds); const extraction = await this.prisma.$transaction(async (tx) => { const created = await tx.extraction.create({ data: { diff --git a/ApiCore-Backend/src/harvests/harvests.service.spec.ts b/ApiCore-Backend/src/harvests/harvests.service.spec.ts index cde3a11..961b1ef 100644 --- a/ApiCore-Backend/src/harvests/harvests.service.spec.ts +++ b/ApiCore-Backend/src/harvests/harvests.service.spec.ts @@ -1,4 +1,4 @@ -import { NotFoundException } from '@nestjs/common'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; import { HarvestsService } from './harvests.service'; import { PrismaService } from '../prisma/prisma.service'; import { AuditLogService } from '../audit/audit-log.service'; @@ -8,6 +8,7 @@ describe('HarvestsService', () => { let service: HarvestsService; let prisma: { harvest: { findUnique: jest.Mock; update: jest.Mock }; + colony: { findMany: jest.Mock }; $transaction: jest.Mock; }; let auditLog: { record: jest.Mock }; @@ -25,6 +26,11 @@ describe('HarvestsService', () => { }; prisma = { harvest: { findUnique: jest.fn(), update: jest.fn() }, + colony: { + findMany: jest + .fn() + .mockResolvedValue([{ id: 'colony-1' }, { id: 'colony-2' }]), + }, $transaction: jest.fn((callback: (txArg: typeof tx) => unknown) => callback(tx), ), @@ -66,6 +72,22 @@ describe('HarvestsService', () => { ]); }); + it('create rejects an unknown colony id instead of failing on the DB foreign-key constraint', async () => { + prisma.colony.findMany.mockResolvedValue([{ id: 'colony-1' }]); + + await expect( + service.create( + { + apiaryId: 'apiary-1', + date: '2026-07-01', + colonyIds: ['colony-1', 'missing-colony'], + }, + 'user-1', + ), + ).rejects.toBeInstanceOf(BadRequestException); + expect(prisma.$transaction).not.toHaveBeenCalled(); + }); + it('update writes an audit log entry with before and after data', async () => { const updated = { ...harvest, notes: 'geändert' }; prisma.harvest.findUnique diff --git a/ApiCore-Backend/src/harvests/harvests.service.ts b/ApiCore-Backend/src/harvests/harvests.service.ts index d66c20a..d000ec1 100644 --- a/ApiCore-Backend/src/harvests/harvests.service.ts +++ b/ApiCore-Backend/src/harvests/harvests.service.ts @@ -1,4 +1,8 @@ -import { Injectable, NotFoundException } from '@nestjs/common'; +import { + BadRequestException, + Injectable, + NotFoundException, +} from '@nestjs/common'; import { PrismaService } from '../prisma/prisma.service'; import { AuditLogService } from '../audit/audit-log.service'; import { WebhooksService } from '../webhooks/webhooks.service'; @@ -35,7 +39,22 @@ export class HarvestsService { return harvest; } + private async assertColoniesExist(colonyIds: string[]): Promise { + const found = await this.prisma.colony.findMany({ + where: { id: { in: colonyIds } }, + select: { id: true }, + }); + const foundIds = new Set(found.map((colony) => colony.id)); + const missing = colonyIds.filter((id) => !foundIds.has(id)); + if (missing.length > 0) { + throw new BadRequestException( + `Unknown colony id(s): ${missing.join(', ')}`, + ); + } + } + async create(dto: CreateHarvestDto, actingUserId: string) { + await this.assertColoniesExist(dto.colonyIds); const harvest = await this.prisma.$transaction(async (tx) => { const created = await tx.harvest.create({ data: { diff --git a/ApiCore-Backend/src/label-templates/label-templates.service.spec.ts b/ApiCore-Backend/src/label-templates/label-templates.service.spec.ts index 098528a..4db5356 100644 --- a/ApiCore-Backend/src/label-templates/label-templates.service.spec.ts +++ b/ApiCore-Backend/src/label-templates/label-templates.service.spec.ts @@ -220,6 +220,75 @@ describe('LabelTemplatesService', () => { expect(pdf.toString('latin1')).toContain('/Image'); }); + it('embeds an image element whose bytes match the declared PNG mime type', async () => { + // Minimal valid 1x1 transparent PNG, widely used as a test fixture. + const onePixelPng = + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII='; + const imageElement: LabelElement = { + id: 'el-img', + type: 'image', + x: 5, + y: 5, + width: 10, + height: 10, + src: `data:image/png;base64,${onePixelPng}`, + }; + prisma.labelTemplate.findUnique.mockResolvedValue({ + ...template, + elements: [imageElement], + }); + prisma.bottlingBatch.findUnique.mockResolvedValue({ + id: 'batch-1', + jarSize: 500, + jarCount: 1, + lotNumber: 'L-001', + date: new Date('2026-06-01T00:00:00.000Z'), + bestBeforeDate: new Date('2028-06-01T00:00:00.000Z'), + honeyBatch: { batchNumber: 'HB-001', honeyType: 'Bluetenhonig' }, + product: { name: 'Glas 500g' }, + }); + + const pdf = await service.renderLabelsPdf('template-1', 'batch-1'); + + // pdfkit always lists /ImageB /ImageC /ImageI in a page's ProcSet + // regardless of whether anything is actually embedded, so a plain + // "/Image" substring check would pass either way - /Subtype /Image is + // the actual embedded-XObject marker. + expect(pdf.toString('latin1')).toMatch(/\/Subtype\s*\/Image\b/); + }); + + it('silently skips an image element whose bytes do not match the declared mime type', async () => { + const mislabeled = Buffer.from('not-a-png').toString('base64'); + const imageElement: LabelElement = { + id: 'el-img', + type: 'image', + x: 5, + y: 5, + width: 10, + height: 10, + src: `data:image/png;base64,${mislabeled}`, + }; + prisma.labelTemplate.findUnique.mockResolvedValue({ + ...template, + elements: [imageElement], + }); + prisma.bottlingBatch.findUnique.mockResolvedValue({ + id: 'batch-1', + jarSize: 500, + jarCount: 1, + lotNumber: 'L-001', + date: new Date('2026-06-01T00:00:00.000Z'), + bestBeforeDate: new Date('2028-06-01T00:00:00.000Z'), + honeyBatch: { batchNumber: 'HB-001', honeyType: 'Bluetenhonig' }, + product: { name: 'Glas 500g' }, + }); + + const pdf = await service.renderLabelsPdf('template-1', 'batch-1'); + + expect(pdf.subarray(0, 4).toString()).toBe('%PDF'); + expect(pdf.toString('latin1')).not.toMatch(/\/Subtype\s*\/Image\b/); + }); + function countPages(pdf: Buffer): number { const matches = pdf.toString('latin1').match(/\/Type\s*\/Page[^s]/g); return matches ? matches.length : 0; diff --git a/ApiCore-Backend/src/label-templates/label-templates.service.ts b/ApiCore-Backend/src/label-templates/label-templates.service.ts index d11bcec..1cb3d20 100644 --- a/ApiCore-Backend/src/label-templates/label-templates.service.ts +++ b/ApiCore-Backend/src/label-templates/label-templates.service.ts @@ -3,6 +3,7 @@ import PDFDocument from 'pdfkit'; import * as QRCode from 'qrcode'; import { PrismaService } from '../prisma/prisma.service'; import { AuditLogService } from '../audit/audit-log.service'; +import { matchesFileSignature } from '../common/file-signature'; import { LabelTemplate } from '../../generated/prisma'; import { CreateLabelTemplateDto } from './dto/create-label-template.dto'; import { UpdateLabelTemplateDto } from './dto/update-label-template.dto'; @@ -62,7 +63,13 @@ function dataUrlToBuffer(dataUrl: string): Buffer | null { if (!match) { return null; } - return Buffer.from(match[2], 'base64'); + const mimeType = match[1] === 'png' ? 'image/png' : 'image/jpeg'; + const buffer = Buffer.from(match[2], 'base64'); + // Same magic-byte check as every other upload path (avatar, documents) - + // a template's embedded image is only ever entered by a label-template + // editor (label-templates:create/update), but there's no reason to hand + // pdfkit's image parser bytes that don't match what they claim to be. + return matchesFileSignature(buffer, mimeType) ? buffer : null; } @Injectable() diff --git a/ApiCore-Backend/src/webhooks/webhooks.service.ts b/ApiCore-Backend/src/webhooks/webhooks.service.ts index 510102a..9fc4a04 100644 --- a/ApiCore-Backend/src/webhooks/webhooks.service.ts +++ b/ApiCore-Backend/src/webhooks/webhooks.service.ts @@ -52,7 +52,7 @@ export class WebhooksService { entityId: subscription.id, action: 'CREATE', userId: actingUserId, - afterData: { ...subscription, secret: undefined }, + afterData: this.omitSecret(subscription), }); return subscription; } @@ -70,10 +70,22 @@ export class WebhooksService { entityId: id, action: 'DELETE', userId: actingUserId, - beforeData: { ...subscription, secret: undefined }, + beforeData: this.omitSecret(subscription), }); } + // Explicit deletion rather than `{ ...subscription, secret: undefined }`: + // that pattern only works because Prisma's JSON serialization happens to + // drop undefined-valued keys today - an explicit delete doesn't depend on + // that being true forever. + private omitSecret( + subscription: WebhookSubscription, + ): Record { + const redacted: Partial = { ...subscription }; + delete redacted.secret; + return redacted; + } + async dispatch(event: WebhookEvent, payload: unknown): Promise { const subscriptions = await this.prisma.webhookSubscription.findMany({ where: { active: true, events: { has: event } },