From c8b86ca9a73cf4041124bce7fad4a5b4b703771f Mon Sep 17 00:00:00 2001 From: duffyduck Date: Fri, 19 Jun 2026 14:14:00 +0200 Subject: [PATCH] Pentest R86: Vertrags-Identifier max 100 + Charset-Whitelist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R86.1 LOW + R86.2 LOW: >999-Zeichen liefen in DB-Overflow (500 statt 400), Attribut-Injection (`foo" onerror=…` ohne umschließenden Tag) überlebte stripHtml. Fix: validateContractIdentifier() (max 100, ^[A-Za-z0-9_\-/. ]{0,100}$) in sanitize.ts, eingehängt in sanitizeContractBody. Wirft ApiError(400, …). Literales Space statt \s → kein CRLF/Tab → kein Header-Injection-Vektor in CSV-/Mail-/PDF-Export. Greift auf alle fünf Identifier-Felder (Provider + Sales-Platform). ContractForm-Inputs bekommen maxLength={100} als UX-Schicht. Co-Authored-By: Claude Opus 4.7 --- .../src/controllers/contract.controller.ts | 11 +++- backend/src/utils/sanitize.ts | 53 +++++++++++++++++++ docs/SECURITY-HARDENING.md | 33 ++++++++++++ docs/todo.md | 15 ++++++ frontend/src/pages/contracts/ContractForm.tsx | 10 ++-- 5 files changed, 115 insertions(+), 7 deletions(-) diff --git a/backend/src/controllers/contract.controller.ts b/backend/src/controllers/contract.controller.ts index 0953d351..5c569e98 100644 --- a/backend/src/controllers/contract.controller.ts +++ b/backend/src/controllers/contract.controller.ts @@ -8,7 +8,7 @@ import * as authorizationService from '../services/authorization.service.js'; import { recordPredecessorFinalReading } from '../services/customer.service.js'; import { ApiResponse, AuthRequest } from '../types/index.js'; import { logChange } from '../services/audit.service.js'; -import { sanitizeContract, sanitizeContractStrict, sanitizeContracts, sanitizeContractsStrict, stripHtml, sanitizeNotes, validateContractDocumentType, validateOptionalIsoDate } from '../utils/sanitize.js'; +import { sanitizeContract, sanitizeContractStrict, sanitizeContracts, sanitizeContractsStrict, stripHtml, sanitizeNotes, validateContractDocumentType, validateOptionalIsoDate, isContractIdentifierField, validateContractIdentifier } from '../utils/sanitize.js'; import { ApiError } from '../utils/apiError.js'; import { canAccessContract } from '../utils/accessControl.js'; import { maybeActivateOnDeliveryConfirmation, withContractDocumentLock } from '../services/contractStatusScheduler.service.js'; @@ -36,7 +36,14 @@ function sanitizeContractBody(body: unknown, parentKey?: string): unknown { if (body === null || body === undefined) return body; if (typeof body === 'string') { if (parentKey && PASSTHROUGH_KEYS.has(parentKey)) return body; - return stripHtml(body); + const stripped = stripHtml(body); + // Pentest 86.1/86.2 (LOW, 2026-06-19): Längen- + Whitelist-Check auf + // Kunden-/Vertrags-/Auftragsnummer-Feldern. validateContractIdentifier + // wirft ApiError(400) bei Verstoß → saubere 400-Antwort statt 500. + if (parentKey && isContractIdentifierField(parentKey)) { + return validateContractIdentifier(stripped, parentKey); + } + return stripped; } if (Array.isArray(body)) return body.map((v) => sanitizeContractBody(v, parentKey)); if (typeof body === 'object') { diff --git a/backend/src/utils/sanitize.ts b/backend/src/utils/sanitize.ts index 9e044ffe..43078c26 100644 --- a/backend/src/utils/sanitize.ts +++ b/backend/src/utils/sanitize.ts @@ -343,6 +343,59 @@ export function validateOptionalIsoDate(raw: unknown, fieldLabel: string): strin return trimmed; } +// Pentest 86.1 + 86.2 (LOW, 2026-06-19): Kunden-/Vertrags-/Auftrags- +// Nummern bei Anbieter und Vertriebsplattform hatten keine Längen- oder +// Zeichen-Validierung. >1000 Zeichen-Strings warfen einen generischen +// 500er (DB-Overflow VARCHAR(191)) statt eines 400ers. Außerdem +// überlebten Attribut-Injection-Payloads wie `foo" onerror="alert(1)` +// die stripHtml-Defense (kein umschließender Tag → kein Match), die +// in PDF-/Mail-Export potentiell aktiv werden könnten. +// +// Whitelist orientiert sich am Vorschlag des Pentesters +// `^[\w\-\/\s\.]*$` – Whitespace ist hier bewusst NUR ein literales +// Space, NICHT `\s` (kein CRLF/Tab → kein Header-Injection-Vektor +// in CSV-/Mail-Exporten). Max 100 Zeichen reicht für jede reale +// Kunden-/Vertrags-Nummer und bleibt deutlich unter dem VARCHAR(191)- +// Limit der DB-Spalte. +const CONTRACT_IDENTIFIER_FIELDS: ReadonlySet = new Set([ + 'customerNumberAtProvider', + 'contractNumberAtProvider', + 'orderNumberAtSalesPlatform', + 'customerNumberAtSalesPlatform', + 'contractNumberAtSalesPlatform', +]); +const CONTRACT_IDENTIFIER_ALLOWED = /^[A-Za-z0-9_\-/. ]{0,100}$/; +const CONTRACT_IDENTIFIER_MAX_LEN = 100; + +export function isContractIdentifierField(key: string): boolean { + return CONTRACT_IDENTIFIER_FIELDS.has(key); +} + +export function validateContractIdentifier( + raw: unknown, + fieldLabel: string, +): string | null { + if (raw == null) return null; + if (typeof raw !== 'string') { + throw new ApiError(400, `${fieldLabel} muss ein Text sein.`); + } + const trimmed = raw.trim(); + if (trimmed === '') return null; + if (trimmed.length > CONTRACT_IDENTIFIER_MAX_LEN) { + throw new ApiError( + 400, + `${fieldLabel} darf maximal ${CONTRACT_IDENTIFIER_MAX_LEN} Zeichen lang sein.`, + ); + } + if (!CONTRACT_IDENTIFIER_ALLOWED.test(trimmed)) { + throw new ApiError( + 400, + `${fieldLabel} enthält unzulässige Zeichen (erlaubt: Buchstaben, Ziffern, Punkt, Bindestrich, Schrägstrich, Unterstrich, Leerzeichen).`, + ); + } + return trimmed; +} + const NOTES_DEFAULT_MAX = 2000; export function sanitizeNotes(raw: unknown, maxLength: number = NOTES_DEFAULT_MAX): string | null { if (raw == null) return null; diff --git a/docs/SECURITY-HARDENING.md b/docs/SECURITY-HARDENING.md index 4d10b56a..6c7ca14a 100644 --- a/docs/SECURITY-HARDENING.md +++ b/docs/SECURITY-HARDENING.md @@ -504,6 +504,39 @@ erneut als „offenes Finding" auftaucht. --- +## 🔒 Runde 86 – Vertrags-Identifier-Validierung + +**Findings (beide LOW):** + +- **R86.1**: Strings >999 Zeichen in `orderNumberAtSalesPlatform` / den + vier verwandten Sales-/Provider-Nummern-Feldern endeten mit + generischem 500 (DB-Overflow `VARCHAR(191)`) statt sauberem 400. +- **R86.2**: Attribut-Injection-Payload `foo" onerror="alert(1)` + (kein umschließender Tag) überlebte `stripHtml`. React escaped + Attribute, aber sobald der Wert in PDF-/Mail-/CSV-Export fließt, + ist es potentiell aktiv. + +**Fix:** `validateContractIdentifier(raw, fieldLabel)` in +[`backend/src/utils/sanitize.ts`](../backend/src/utils/sanitize.ts): + +- Max-Länge 100 Zeichen (deutlich unter VARCHAR(191)). +- Whitelist `^[A-Za-z0-9_\-/. ]{0,100}$` – Buchstaben, Ziffern, + Punkt, Bindestrich, Schrägstrich, Unterstrich und literales + Leerzeichen. Bewusst NICHT `\s` (kein CRLF/Tab → kein + Header-Injection-Vektor in CSV-/Mail-Exporten). +- Bei Verstoß: `ApiError(400, …)` mit konkreter Fehlermeldung + statt 500. +- Eingehängt in `sanitizeContractBody` → läuft automatisch für alle + fünf Identifier-Felder (`customerNumberAtProvider`, + `contractNumberAtProvider`, `orderNumberAtSalesPlatform`, + `customerNumberAtSalesPlatform`, `contractNumberAtSalesPlatform`) + bei jedem Create/Update. +- Frontend: `maxLength={100}` als zusätzliche UX-Schicht im + ContractForm – Server-seitige Validierung bleibt die einzige + Wahrheit, das HTML-Attribut spart nur den unnötigen Round-Trip. + +--- + ## 🧭 Wann ist „dicht" dicht? 100 % gibt es nicht. Erreicht ist: diff --git a/docs/todo.md b/docs/todo.md index 5f70ebc8..a352f047 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -97,6 +97,21 @@ isolierte Instanz (keine Multi-Tenancy im Code), Provisioning + Abrechnung ## ✅ Erledigt +- [x] **🔒 Pentest R86 – Vertrags-Identifier härten** + - R86.1 (LOW): >999-Zeichen-Strings auf Kunden-/Vertrags-/ + Auftragsnummer warfen 500 (DB-Overflow `VARCHAR(191)`) statt 400. + - R86.2 (LOW/INFO): Attribut-Injection ohne umschließenden Tag + (`foo" onerror=…`) überlebte `stripHtml` – kein Risiko in der React- + UI, aber relevant für PDF/Mail/CSV-Export. + - Fix: zentraler `validateContractIdentifier()` in `sanitize.ts` + mit Max-100 und Whitelist `^[A-Za-z0-9_\-/. ]{0,100}$`. Bewusst + literales Space statt `\s`, damit kein CRLF/Tab passiert (Header- + Injection). Wirft `ApiError(400, …)` mit klarer Meldung. + - Eingehängt in `sanitizeContractBody` → läuft automatisch für alle + fünf Identifier-Felder bei Create/Update. ContractForm bekommt + `maxLength={100}` als UX-Schicht. Doku in + `docs/SECURITY-HARDENING.md` § Runde 86. + - [x] **🆕 Vertrag: Auftragsnummer bei Vertriebsplattform** - Neues optionales Feld `Contract.orderNumberAtSalesPlatform` (`VARCHAR(191) NULL`), Migration diff --git a/frontend/src/pages/contracts/ContractForm.tsx b/frontend/src/pages/contracts/ContractForm.tsx index c3a877f2..02c8ccc5 100644 --- a/frontend/src/pages/contracts/ContractForm.tsx +++ b/frontend/src/pages/contracts/ContractForm.tsx @@ -956,11 +956,11 @@ export default function ContractForm() { options={availableTariffs.map((t) => ({ value: t.id, label: t.name }))} disabled={!selectedProviderId} /> - - - - - + + + + +