16 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 07-dkv-fleet-module | 2026-06-27T15:15:00Z | standard | 44 |
|
|
issues_found |
Phase 07: Code Review Report
Reviewed: 2026-06-27T15:15:00Z Depth: standard Files Reviewed: 44 Status: issues_found
Summary
The DKV Fleet module is broadly well-structured. Security-sensitive concerns (credential encryption, traversal guards, credential masking in logs) are handled carefully throughout. The multi-layer architecture (controller → service → providers) is clean and the tenant-scoping pattern is consistent.
Three blockers were found: a field-name mismatch between the CSV import API response and the frontend consumer that makes the import count permanently display as undefined; a cron expression construction that is silently incorrect for any polling interval above 59 minutes; and an IMAP client connection leak when the mailbox lock call fails before the try/finally cleanup block. Five warnings cover a nodemailer transport resource leak, missing numeric coercion on query-string pagination params, dead-code error handling in the datetime formatter, an unsanitised Exchange UID reaching the filesystem write path, and the absence of a file-size limit on the CSV upload endpoint.
Critical Issues
CR-01: CSV import count field mismatch — import count always undefined
File: apps/api/src/dkv/dkv.service.ts:501
Also affects: apps/web/src/lib/dkv-api.ts:238, apps/web/src/app/(portal)/modules/dkv-fleet/settings/components/CsvImportButton.tsx:72
Issue: DkvService.importVehiclesCsv returns { imported: number, mode: string }. The frontend API wrapper importVehiclesCsv declares its return type as Promise<{ count: number }> and both call sites read result.count. At runtime result.count is always undefined because the JSON key is imported, not count. The success toast in both CsvImportButton (result.count) and VehicleTable.handleImported (count arg forwarded from CsvImportButton) will always display "undefined Fahrzeuge importiert".
Fix:
Either rename the backend field:
// dkv.service.ts line 501
return { count: vehicles.length, mode };
Or update the frontend type and usage to match the existing backend field:
// dkv-api.ts
export async function importVehiclesCsv(...): Promise<{ imported: number; mode: string }> { ... }
// CsvImportButton.tsx line 72
setImportSuccess(t('importSuccess', { count: result.imported }));
onImported(result.imported);
CR-02: Cron expression silently incorrect for pollIntervalMin > 59
File: apps/api/src/dkv/dkv-scheduler.service.ts:105
Issue: The cron expression is built as:
const cronExpr = `*/${intervalMin} * * * *`;
In standard cron the minute field only accepts values 0–59. When intervalMin is 60 or above, */60 and */120 both resolve to "fire at minute 0 only" — i.e., once per hour — instead of the intended interval. The DkvConfigDto has no @Max guard on pollIntervalMin, so an admin can set any value. An interval of 120 minutes would behave identically to 60 minutes with no error, and an interval of 90 minutes would silently switch to hourly. Depending on the cron package, values > 59 may also throw at job-creation time, causing setInterval to propagate an unhandled exception from the controller's saveConfig handler (which has no try/catch around the dkvScheduler.setInterval() call).
Fix — correct multi-hour intervals using the hours field:
setInterval(intervalMin: number, tenantId?: string): void {
// ...
let cronExpr: string;
if (intervalMin < 60) {
cronExpr = `*/${intervalMin} * * * *`; // e.g. */15 * * * *
} else {
const hours = Math.floor(intervalMin / 60);
cronExpr = `0 */${hours} * * *`; // e.g. 0 */2 * * *
}
// ...
}
Also add @Max(1440) to DkvConfigDto.pollIntervalMin and document that intervals must be a factor of 60 for sub-hourly, or a factor of 24 for hourly+ to behave predictably.
CR-03: IMAP connection not closed when mailbox lock fails
File: apps/api/src/dkv/providers/imap.provider.ts:106-176
Issue: client.connect() (line 106) and client.getMailboxLock() (line 107) are called before the try/finally block that handles cleanup (lines 172–175). If getMailboxLock throws (e.g., the configured folder does not exist, or the server rejects the command), the finally block is never entered and neither lock.release() nor client.logout() is called. The ImapFlow connection is left open and leaked. This will exhaust connection limits on the mail server under repeated failures.
// Current — getMailboxLock is outside try:
await client.connect();
const lock = await client.getMailboxLock(config.folder || 'INBOX');
try { ... } finally { lock.release(); await client.logout(); }
Fix — wrap the lock acquisition inside the try block:
await client.connect();
let lock: Awaited<ReturnType<typeof client.getMailboxLock>> | null = null;
try {
lock = await client.getMailboxLock(config.folder || 'INBOX');
// ... search and download logic
} finally {
lock?.release();
await client.logout();
}
Warnings
WR-01: nodemailer transport not closed — connection pool leak
File: apps/api/src/dkv/dkv-mail.service.ts:56-106
Issue: nodemailer.createTransport() is called on every sendExportEmail invocation (intentional — per Pitfall 3 mitigation), but the resulting transport is never closed. nodemailer transports keep an internal SMTP connection pool alive. With 3-retry exponential backoff, up to three transports can be created per invoice. Over time (especially with SMTP errors triggering repeated sends) these accumulate and can exhaust OS socket limits or hit server connection caps.
Fix — close the transport in a finally block:
const transport = nodemailer.createTransport({ ... });
try {
await transport.sendMail({ ... });
this.logger.log(`DKV export email sent to ${recipient}: ${filename}`);
} catch (error) {
this.logger.error(`Failed to send DKV export to ${recipient} (file: ${filename}): ${(error as Error).message}`);
throw error;
} finally {
transport.close();
}
WR-02: Pagination query params not coerced to numbers — @IsInt() silently fails
File: apps/api/src/dkv/dto/dkv-history.dto.ts:18-29
Issue: page and limit are typed as number with @IsInt() validators. HTTP query parameters arrive as strings ("1", "20"). Without @Type(() => Number) from class-transformer, the values are never coerced to numbers before validation. If the global ValidationPipe does not enable enableImplicitConversion, @IsInt() will reject the string inputs and the endpoint will return HTTP 400 whenever page or limit are supplied. If ValidationPipe strips unknown types, the defaults kick in silently — masking the broken validation.
Fix:
import { Type } from 'class-transformer';
export class DkvHistoryQueryDto {
@IsOptional()
@IsInt()
@Min(1)
@Type(() => Number)
page?: number;
@IsOptional()
@IsInt()
@Min(1)
@Max(100) // also add an upper bound
@Type(() => Number)
limit?: number;
}
WR-03: formatDateTime never catches — invalid dates produce "NaN.NaN.NaN, NaN:NaN Uhr"
File: apps/web/src/app/(portal)/modules/dkv-fleet/components/InvoiceHistoryTable.tsx:28-39
Issue: The try/catch in formatDateTime is dead code. new Date(invalidString) does not throw — it returns an Invalid Date object. Subsequent calls to d.getDate() etc. return NaN. The catch branch is never reached, and malformed datumZeit values from the API silently render as "NaN.NaN.NaN, NaN:NaN Uhr" in the history table.
Fix — check for invalid date explicitly:
function formatDateTime(isoString: string): string {
const d = new Date(isoString);
if (isNaN(d.getTime())) return isoString; // fallback to raw string
const day = String(d.getDate()).padStart(2, '0');
const month = String(d.getMonth() + 1).padStart(2, '0');
const year = d.getFullYear();
const hours = String(d.getHours()).padStart(2, '0');
const minutes = String(d.getMinutes()).padStart(2, '0');
return `${day}.${month}.${year}, ${hours}:${minutes} Uhr`;
}
WR-04: Exchange UniqueId can contain path characters in generated export filename
File: apps/api/src/dkv/dkv-export.service.ts:139-143
Also affects: apps/api/src/dkv/dkv.service.ts:619-624
Issue: When a DKV invoice email subject does not match the expected invoice number pattern, _extractInvoiceNumber falls back to email-${String(uid)}. For IMAP the UID is a numeric integer (safe). For Exchange, uid is String(item.Id?.UniqueId) — an EWS UniqueId which is base64-encoded and can contain +, /, and =. The / character causes path.join(userFilesDir, 'DKV_YYYY-MM_email-AA+Bx/CCdef.xlsx') to resolve into a subdirectory of user-files/. fs.writeFileSync then fails (subdirectory doesn't exist), and the error propagates through _processAttachment. While the immediate impact is a failed write (not arbitrary overwrite), the uncontrolled path component from an external data source should be sanitised before reaching the filesystem.
The getExportFile read-time guard (!/^DKV_[\w\-]+\.xlsx$/.test(filename)) would reject such a filename at download time, but the write-time path has no equivalent guard.
Fix — sanitise the rechnungsnummer before building the filename:
// In _extractInvoiceNumber, sanitise the fallback:
return `email-${String(uid).replace(/[^a-zA-Z0-9\-]/g, '_')}`;
// Or alternatively, in writeAndPrune, sanitise before building the path:
const safeName = `${invoiceMonth}_${rechnungsnummer}`.replace(/[^a-zA-Z0-9\-_]/g, '_');
const filename = `DKV_${safeName}.xlsx`;
WR-05: No file size limit on CSV multipart upload
File: apps/api/src/dkv/dkv.controller.ts:207-225
Issue: @UseInterceptors(FileInterceptor('file')) uses multer's default memory storage with no size limit. An admin could upload an arbitrarily large file, buffering its entire content in Node.js heap before importVehiclesCsv parses it. The vehicle CSV parsing is synchronous and unbounded; a 500 MB file would OOM the process.
Fix — add a file size limit:
@UseInterceptors(FileInterceptor('file', {
limits: { fileSize: 5 * 1024 * 1024 }, // 5 MB — generous for any realistic vehicle list
}))
Info
IN-01: Dead code — downloadExport try/catch is a no-op
File: apps/api/src/dkv/dkv.controller.ts:143-157
Issue: Both branches of the catch block unconditionally re-throw error:
} catch (error) {
if (error instanceof NotFoundException || error instanceof BadRequestException) {
throw error; // branch A
}
throw error; // branch B — identical
}
The if condition has no effect. The entire try/catch wrapper adds no value and should be removed to reduce noise.
Fix: Remove the try/catch wrapper entirely; let NestJS exception filters handle the propagation naturally.
IN-02: UpdateVehicleDto JSDoc documents PATCH but controller uses PUT
File: apps/api/src/dkv/dto/dkv-vehicle.dto.ts:46
Issue: The JSDoc comment reads "Used for: PATCH /dkv/vehicles/:id" but DkvController registers the endpoint as @Put('vehicles/:id'). This is a documentation/contract discrepancy — REST semantics differ between PUT (full replacement) and PATCH (partial update). The DTO only validates individual optional fields, which aligns with PATCH semantics, not PUT.
Fix: Either change the JSDoc to "PUT" to match the implemented decorator, or change the decorator to @Patch and document that it performs a partial update.
IN-03: Settings page activeTab state for 'vehicles' is never set
File: apps/web/src/app/(portal)/modules/dkv-fleet/settings/page.tsx:17-53
Issue: activeTab is declared as ActiveTab = 'inbox' | 'vehicles' but the "Fahrzeuge" tab is rendered as a Next.js <Link> that navigates to a separate route (/modules/dkv-fleet/vehicles). The setActiveTab('vehicles') call never executes. As a result, tabCls('vehicles') never returns the active style class, and activeTab === 'inbox' is always true. The useState and the 'vehicles' branch in tabCls are dead code.
Fix: Remove activeTab state entirely. Use usePathname() to derive active styling for both tabs:
const pathname = usePathname();
const tabCls = (href: string) =>
pathname === href || pathname.startsWith(href)
? 'border-b-2 border-primary text-foreground font-medium px-4 py-2 text-sm'
: 'text-muted-foreground hover:text-foreground px-4 py-2 text-sm';
IN-04: Parser logic fully duplicated between service and validation script
File: apps/api/src/dkv/dkv-parser.validate.ts:17-148
Also: apps/api/src/dkv/dkv-parser.service.ts
Issue: dkv-parser.validate.ts contains a complete copy of parseDE, parseSingleTxLine, parseMultiTxColumnar, parseTransactionRows, and parseDkvText. Any bug fix or format change must be applied in both places. The functions are already exported/importable from the service file.
Fix: Import the logic from the service rather than duplicating it. Since the validation script uses --experimental-strip-types and the service file uses NestJS decorators (which won't execute outside the framework), extract the pure parsing functions into a separate dkv-parser.utils.ts file and import from there in both the service and the validation script.
Reviewed: 2026-06-27T15:15:00Z Reviewer: Claude (gsd-code-reviewer) Depth: standard