Pentest R92: Strict-400 für accountId auf Vertrags-Endpunkten
R91-Fix war silent-undefined bei invaliden Werten – accountId=abc
auf Vertrags-Endpunkten brach die Mailbox-Isolation (Mails aus
allen Postfächern statt 400). Pentester R92 hat zu Recht
Strict-400 vorgeschlagen.
Helper parsePositiveIntQuery() bekommt { required } option:
- optional (default): fehlend → undefined (kein Filter), invalid → 400
- required: fehlend ODER invalid → 400
Vertrags-Endpunkte (Emails + Folder-Counts) auf required gestellt.
Customer-/Trash-Endpunkte bleiben optional (Cross-Mailbox-View ist
legitim), aber invalid → 400. Frontend hat eh enabled-Guards.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -41,22 +41,55 @@ function parseDateParam(v: unknown): Date | undefined {
|
||||
return isNaN(d.getTime()) ? undefined : d;
|
||||
}
|
||||
|
||||
// Pentest 91.1 (LOW, 2026-06-21): `accountId=abc` (→ parseInt = NaN) und
|
||||
// `accountId=` (leer string ist truthy nach `req.query.accountId ? …`)
|
||||
// kamen vorher als `NaN` im Service an. `if (NaN)` ist falsy → der
|
||||
// stressfreiEmailId-Filter wurde komplett übersprungen, und der Vertrag
|
||||
// zeigte Mails aus ALLEN Postfächern.
|
||||
// Pentest 91.1 (LOW, 2026-06-21): `accountId=abc` → `parseInt` = `NaN`
|
||||
// → der Ternary gab `NaN` an den Service. `if (NaN)` ist falsy → der
|
||||
// Postfach-Filter fiel weg, und der Vertrag zeigte Mails aus ALLEN
|
||||
// Postfächern.
|
||||
//
|
||||
// Helper akzeptiert nur positive Ganzzahlen; alles andere → undefined,
|
||||
// das im Service den Filter wegfallen lässt. Wenn wir wollten dass
|
||||
// invalide Werte 400 werfen statt silent ignoriert zu werden, müssten
|
||||
// wir das pro Endpunkt entscheiden – die Semantik „Filter weglassen"
|
||||
// ist hier in Ordnung, weil der `customerId`-/`contractId`-Constraint
|
||||
// im `where` weiterhin greift (kein Cross-Customer-Leak).
|
||||
function parsePositiveIntParam(v: unknown): number | undefined {
|
||||
if (typeof v !== 'string' || v.trim() === '') return undefined;
|
||||
// Pentest 92 (LOW, 2026-06-21): Bei `accountId=abc` auf Vertrags-
|
||||
// Endpunkten reichte das silent-undefined nicht – die Mailbox-Isolation
|
||||
// brach (man sah Mails aus allen Postfächern statt 0). Strict-400, weil
|
||||
// Verträge per Design IMMER ein bestimmtes Postfach meinen.
|
||||
//
|
||||
// Helper hat zwei Modi:
|
||||
// - default (optional): fehlend/leer → undefined (kein Filter)
|
||||
// invalid → 400
|
||||
// - { required: true }: fehlend/leer → 400
|
||||
// invalid → 400
|
||||
// Bei 400 schreibt der Helper direkt die Response und gibt `null`
|
||||
// zurück; der Caller bricht dann mit `return` ab.
|
||||
function parsePositiveIntQuery(
|
||||
v: unknown,
|
||||
fieldLabel: string,
|
||||
res: Response,
|
||||
options?: { required?: boolean },
|
||||
): number | undefined | null {
|
||||
const absent = v === undefined || v === '' || (typeof v === 'string' && v.trim() === '');
|
||||
if (absent) {
|
||||
if (options?.required) {
|
||||
res.status(400).json({
|
||||
success: false,
|
||||
error: `${fieldLabel} ist erforderlich (positive Ganzzahl).`,
|
||||
} as ApiResponse);
|
||||
return null;
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
if (typeof v !== 'string') {
|
||||
res.status(400).json({
|
||||
success: false,
|
||||
error: `${fieldLabel} muss als Zahl übergeben werden.`,
|
||||
} as ApiResponse);
|
||||
return null;
|
||||
}
|
||||
const n = parseInt(v, 10);
|
||||
if (!Number.isFinite(n) || n < 1 || !Number.isInteger(n)) return undefined;
|
||||
if (!Number.isFinite(n) || n < 1 || !Number.isInteger(n)) {
|
||||
res.status(400).json({
|
||||
success: false,
|
||||
error: `${fieldLabel} muss eine positive Ganzzahl sein.`,
|
||||
} as ApiResponse);
|
||||
return null;
|
||||
}
|
||||
return n;
|
||||
}
|
||||
|
||||
@@ -65,7 +98,10 @@ export async function getEmailsForCustomer(req: AuthRequest, res: Response): Pro
|
||||
try {
|
||||
const customerId = parseInt(req.params.customerId);
|
||||
if (!(await canAccessCustomer(req, res, customerId))) return;
|
||||
const stressfreiEmailId = parsePositiveIntParam(req.query.accountId);
|
||||
// Customer-Inbox: accountId ist legitim optional (cross-mailbox-Ansicht
|
||||
// ist erwünscht), aber invalide Werte → 400.
|
||||
const stressfreiEmailId = parsePositiveIntQuery(req.query.accountId, 'accountId', res);
|
||||
if (stressfreiEmailId === null) return;
|
||||
const folder = req.query.folder as string | undefined; // INBOX oder SENT
|
||||
const limit = req.query.limit ? parseInt(req.query.limit as string) : 50;
|
||||
const offset = req.query.offset ? parseInt(req.query.offset as string) : 0;
|
||||
@@ -109,7 +145,11 @@ export async function getEmailsForContract(req: AuthRequest, res: Response): Pro
|
||||
const contractId = parseInt(req.params.contractId);
|
||||
if (!(await canAccessContract(req, res, contractId))) return;
|
||||
const folder = req.query.folder as string | undefined; // INBOX oder SENT
|
||||
const stressfreiEmailId = parsePositiveIntParam(req.query.accountId);
|
||||
// Vertrags-Endpunkte sind per Design IMMER pro Postfach – fehlt
|
||||
// accountId, ist die Abfrage semantisch ungültig. Strict-400.
|
||||
// Frontend hat eh ein `enabled: !!selectedAccountId`-Guard.
|
||||
const stressfreiEmailId = parsePositiveIntQuery(req.query.accountId, 'accountId', res, { required: true });
|
||||
if (stressfreiEmailId === null || stressfreiEmailId === undefined) return;
|
||||
const limit = req.query.limit ? parseInt(req.query.limit as string) : 50;
|
||||
const offset = req.query.offset ? parseInt(req.query.offset as string) : 0;
|
||||
|
||||
@@ -267,7 +307,10 @@ export async function getContractFolderCounts(req: AuthRequest, res: Response):
|
||||
try {
|
||||
const contractId = parseInt(req.params.contractId);
|
||||
if (!(await canAccessContract(req, res, contractId))) return;
|
||||
const stressfreiEmailId = parsePositiveIntParam(req.query.accountId);
|
||||
// Wie getEmailsForContract: Postfach ist required (sonst zeigt der
|
||||
// Badge eine andere Zahl als die Liste).
|
||||
const stressfreiEmailId = parsePositiveIntQuery(req.query.accountId, 'accountId', res, { required: true });
|
||||
if (stressfreiEmailId === null || stressfreiEmailId === undefined) return;
|
||||
|
||||
const counts = await cachedEmailService.getFolderCountsForContract(contractId, stressfreiEmailId);
|
||||
|
||||
@@ -915,8 +958,12 @@ export async function getTrashEmails(req: AuthRequest, res: Response): Promise<v
|
||||
try {
|
||||
const customerId = parseInt(req.params.customerId);
|
||||
if (!(await canAccessCustomer(req, res, customerId))) return;
|
||||
const stressfreiEmailId = parsePositiveIntParam(req.query.accountId);
|
||||
const contractId = parsePositiveIntParam(req.query.contractId);
|
||||
// Trash auf Kundenebene: Filter sind optional (Cross-Mailbox-Trash-
|
||||
// View ist legitim), invalide Werte → 400.
|
||||
const stressfreiEmailId = parsePositiveIntQuery(req.query.accountId, 'accountId', res);
|
||||
if (stressfreiEmailId === null) return;
|
||||
const contractId = parsePositiveIntQuery(req.query.contractId, 'contractId', res);
|
||||
if (contractId === null) return;
|
||||
|
||||
const emails = await cachedEmailService.getTrashEmails(customerId, {
|
||||
stressfreiEmailId,
|
||||
@@ -938,8 +985,10 @@ export async function getTrashCount(req: AuthRequest, res: Response): Promise<vo
|
||||
try {
|
||||
const customerId = parseInt(req.params.customerId);
|
||||
if (!(await canAccessCustomer(req, res, customerId))) return;
|
||||
const stressfreiEmailId = parsePositiveIntParam(req.query.accountId);
|
||||
const contractId = parsePositiveIntParam(req.query.contractId);
|
||||
const stressfreiEmailId = parsePositiveIntQuery(req.query.accountId, 'accountId', res);
|
||||
if (stressfreiEmailId === null) return;
|
||||
const contractId = parsePositiveIntQuery(req.query.contractId, 'contractId', res);
|
||||
if (contractId === null) return;
|
||||
|
||||
const count = await cachedEmailService.getTrashCount(customerId, {
|
||||
stressfreiEmailId,
|
||||
|
||||
@@ -97,6 +97,23 @@ isolierte Instanz (keine Multi-Tenancy im Code), Provisioning + Abrechnung
|
||||
|
||||
## ✅ Erledigt
|
||||
|
||||
- [x] **🔒 Pentest R92 – Strict-400 für accountId auf Vertrags-Endpunkten**
|
||||
- R91-Fix war silent-undefined bei invaliden Werten: `accountId=abc`
|
||||
auf `GET /contracts/:id/emails` ergab "kein Filter" → Mailbox-
|
||||
Isolation brach (alle Postfächer sichtbar). Pentester R92: per
|
||||
Design sind Vertrags-Endpunkte immer pro Postfach, also strict-400.
|
||||
- Fix: `parsePositiveIntQuery(v, label, res, { required? })`
|
||||
ersetzt den alten silent-Helper. Modes:
|
||||
- default (optional): fehlend/leer → `undefined` (kein Filter),
|
||||
invalid → 400
|
||||
- `{ required: true }`: fehlend/leer **oder** invalid → 400
|
||||
- Verteilung:
|
||||
- Contract-Emails, Contract-Folder-Counts: `{ required: true }`
|
||||
- Customer-Emails, Trash, Trash-Count: optional (Cross-Mailbox-
|
||||
View ist legitim), invalid → 400
|
||||
- Frontend hat schon ein `enabled: !!selectedAccountId`-Guard auf
|
||||
den Vertrags-Queries – kein UX-Bruch.
|
||||
|
||||
- [x] **🔒 Pentest R91 – NaN-Bypass auf accountId-Query-Param**
|
||||
- R91.1 (LOW): `accountId=abc` → `parseInt('abc')` = `NaN` → der
|
||||
Ternary im Controller gab `NaN` an den Service, `if (NaN)` ist
|
||||
|
||||
Reference in New Issue
Block a user