diff --git a/docs/codebase-audit-findings-2026-06-06.md b/docs/codebase-audit-findings-2026-06-06.md new file mode 100644 index 0000000..1acef78 --- /dev/null +++ b/docs/codebase-audit-findings-2026-06-06.md @@ -0,0 +1,544 @@ +# Codebase Audit — Full Findings Catalog (2026-06-06) + +Generated from the 16-dimension discovery sweep. 84 findings (high 5, medium 31, low 48). + +## Dimension summaries + +**svc-errors** — test2 + +**routes** — Route handlers in src/routes/admin/* are largely consistent and follow the documented patterns well: nearly every route uses a requirePermission/requireAnyPermission/requireAuth guard, validates bodies via parseBody(ZodSchema, ...) with the uniform `if ("error" in parsed) return error(reply, parsed.error, 400)` shape, uses parseId for numeric route params in most files, and logs audit entries on create/update/delete with old/new values. The warehouse, customers, invoices, projects, roles, and auth/totp files are exemplary. The real issues are: (1) a genuine authorization/visibility bug in plan.ts where the admin check reads a non-existent field; (2) a recurring style violation where roughly a third of files send raw `reply.send({ success: true, ... })` instead of the success()/paginated() helpers that CLAUDE.md mandates; (3) two security-relevant session-termination mutations with no audit logging; and (4) inconsistent ID parsing (raw parseInt vs the parseId helper) in trips.ts and sessions.ts. HTTP status codes are generally correct (201 on create, 404 not-found, 409 conflict, 403 permission), with one cluster of not-found/validation responses in project-files.ts falling back to the default 400. + +**schemas** — The 21 Zod schema files in src/schemas/* are consistently organized (one file per entity, Create/Update pairs, z.infer type exports, Czech user-facing messages — all intentional and good). Every route body is validated via the shared parseBody helper (src/schemas/common.ts) — coverage is complete; the handful of raw `request.body` reads (attendance.ts, orders.ts, projects.ts, project-files.ts) either re-dispatch to parseBody with branch-specific schemas or do narrow manual checks, so there is no unvalidated-body gap. The dominant real issue is massive duplication of three coercion idioms — a number-from-form `z.union([z.number(),z.string()]).transform(v=>Number(v))`, a nullable variant with `z.null()`, and a boolean-from-form `z.preprocess(v=>v===true||v===1||v==="1", z.boolean())` — copy-pasted ~150+ times across files instead of living as shared helpers in common.ts. Secondary issues: the number coercion is unsafe (no NaN guard in most places, so "abc" silently becomes NaN), email validation is applied inconsistently (only users/profile, not customers/warehouse/settings), error-message language is mixed (Czech vs English "Must be a valid number"), no schemas use strict object mode so unknown keys are silently stripped, and the two inline route schemas use Zod-3-deprecated `.strict()`/`.passthrough()` rather than Zod 4's `z.strictObject()`/`z.looseObject()`. Severity is mostly low/medium: these are maintainability and robustness gaps, not security holes (the app is on Zod 4.3.6, so the modern helpers are available). + +**logging** — Overall the codebase handles errors deliberately: routes use Fastify's pino logger (request.log.*/app.log.*), the global error handler logs unhandled exceptions, audit failures are logged and treated as non-fatal, and a couple of catch blocks (auth.ts verifyAccessToken, attendance/leave duplicate-record handling) are intentionally selective with good comments. The two real systemic issues are: (1) the NAS file/financials managers (src/services/nas-file-manager.ts, src/services/nas-financials-manager.ts) contain ~25 empty/silent catch blocks that swallow genuine filesystem operation failures (delete/mkdir/write/rename) with zero logging, returning only a generic Czech string — directly violating the CLAUDE.md "never silently swallow errors" rule and making NAS issues undebuggable; and (2) all service-layer error logging uses console.* instead of the configured pino logger, so those messages bypass structured logging and the production log-level config. A secondary correctness smell: markOverdueInvoices swallows DB-update failures and returns void, so its route caller treats a failed update as success. Frontend error handling is consistent (alert.error with Czech fallback); a few best-effort .catch(()=>{}) swallows on background calls are defensible but undocumented. + +**prisma** — Solid Prisma usage overall. Real issues in findings. + +**security** — Auth and crypto fundamentals are solid: refresh tokens are SHA-256-hashed with rotation + reuse detection (replaced_at/replaced_by_hash + FOR UPDATE), TOTP has counter-based replay protection, password comparison is timing-safe (dummy bcrypt hash on user-not-found/locked/inactive), TOTP secrets are AES-256-GCM encrypted, account lockout + per-route rate limits exist, bcrypt cost is 12, and JWT verification pins HS256. Contrary to CLAUDE.md "known issue #5", HTML sanitization for invoice notes, quotation scope, and order notes is NOT missing: DOMPurify (jsdom) is applied at all three server-side PDF routes plus a regex-based cleanQuillHtml second pass, and both frontend render sites (InvoiceDetail, OrderDetail) sanitize before dangerouslySetInnerHTML. NAS file access has strong path-traversal defenses (rejects "..", null bytes, confines to base, rejects symlink components, MIME/extension allowlist). The real gaps are narrower: the TOTP-code verification step does not increment failed-login counters (relies only on a 5/min rate limit), HSTS lacks preload, and the server-side invoice/offer PDF endpoints serve attacker-influenced (but sanitized) HTML as same-origin text/html. No SQL injection found — all $queryRaw uses are parameterized tagged templates. + +**dates** — Date handling splits into two well-reasoned, internally-consistent regimes plus a set of genuine off-by-one bugs at the boundary between them. (1) The plan module (src/services/plan.service.ts, src/admin/pages/PlanWork.tsx, usePlanWork.ts, PlanGrid.tsx) deliberately does ALL date-only arithmetic in UTC (setUTCDate/getUTCDate, toISOString().slice(0,10), proper ISO-8601 week calc). Because every column it touches is `@db.Date` (stored at UTC-midnight by Prisma), this is correct and stable — explicitly documented at plan.service.ts:63-67 and 342-352. Do not "fix" it. (2) The attendance/leave server code constructs `@db.Date` writes at LOCAL noon (new Date(y,m,d,12,0,0)), which is the robust convention. The real problems: a repeated frontend pattern `new Date().toISOString().split("T")[0]` for "today" defaults computes the UTC date, giving yesterday in the late-evening Prague window; the invoices/received-invoices/trips edit forms round-trip API date strings through `new Date(x).toISOString().split(...)` instead of the existing string-slice helper `normalizeDateStr`, risking a shift; trips' raw API datetime string is fed into a native (blank on mobile); markOverdueInvoices compares a `@db.Date` due_date against full `new Date()` so invoices flip to "overdue" on their due day; and getCzechDate uses a non-ISO local week formula that disagrees with PlanWork's ISO week. A correct local-date string helper (utils/date.ts localDateStr, and frontend normalizeDateStr) already exists but isn't used in the offending spots. + +**ts-safety** — Both tsconfigs enable `strict: true` and there are zero `@ts-ignore`/`@ts-nocheck` directives, so the baseline is good. The real risk is concentrated, not diffuse: 77 `any` occurrences cluster in the work-plan stack (plan.service.ts, routes/admin/plan.ts, hooks/usePlanWork.ts) and a few PDF/HTML builders. The single most important finding is a latent authorization-scoping bug masked by an `any`-typed parameter in plan.ts (`isAdminLike` reads `authData.role`, but `AuthData` only has `roleName`), which makes admins always scope to their own plan rows. Systemically, ~47 of 107 exported service functions have no explicit return type, which forces `(result as any).status` casts in route handlers (projects.ts, quotations.ts) because the inferred error union isn't uniform. There is also a genuinely duplicated/contradictory `PaginationMeta` shape defined three times with diverging fields. Non-null assertions are mostly the defensible `request.authData!` post-auth pattern and are not a concern. + +**rq-data** — The frontend data layer is built on a clean, idiomatic foundation: every read is a `queryOptions()` factory in `src/admin/lib/queries/*` (TanStack Query v5 best practice), data flows through a thin `apiAdapter.ts` (jsonQuery/paginatedJsonQuery) that unwraps the `{success,data}` envelope and throws on error, and a purpose-built `useApiMutation` hook (lib/queries/mutations.ts) wraps `useMutation` + apiFetch + broad invalidation exactly as the docs recommend. The factory pattern is consistently applied for queries and `useApiMutation` is adopted in 34 files. The real weakness is on the WRITE side: a large minority of mutations bypass `useApiMutation` and hand-roll `apiFetch` + `await response.json()` + manual `invalidateQueries` + `try/catch alert("Chyba připojení")` (14 files, ~27 sites), duplicating the exact logic the hook exists to centralize. Within that hand-rolled code the broad-domain invalidation convention from CLAUDE.md is violated in OfferDetail.tsx (uses narrow `["offers","list"]` while Offers.tsx uses broad `["offers"]`). useAttendanceAdmin.ts runs an entirely parallel data system (manual fetchData + setTimeout(300) instead of query factories). Query-key conventions themselves are sound; minor smells are a duplicated `require2FAOptions` factory and an unused deprecated alias. None of these are correctness bugs — they are consistency/maintainability gaps. Best-practice basis: TanStack Query v5 docs (/tanstack/query) confirm prefix-matching invalidation (validating the broad-key convention) and the useMutation→onSuccess→invalidateQueries pattern that useApiMutation implements. + +**react** — The frontend is mostly modern and disciplined: data flows through TanStack Query via lib/queries/*, lazy-loading is consistent for every page in AdminApp.tsx, and there are two solid reusable modal primitives (FormModal, ConfirmModal) used in 34 files. The real issues are concentrated and concrete: (1) one large hook (useAttendanceAdmin.ts, ~830 lines) opts out of TanStack Query and hand-rolls fetching with useEffect/useState, complete with silent empty catches and no race-condition guard — the single biggest consistency/best-practice gap; (2) two editable, mutable lists use the array index as React key (OrderConfirmationModal items, OfferDetail scope sections), which the React docs flag as a correctness bug because rows are deleted/reordered while holding controlled-input state; (3) modal usage is inconsistent — alongside FormModal/ConfirmModal, ~7 components hand-roll the admin-modal-overlay markup with diverging accessibility (DashProfile/DashQuickActions omit role="dialog"/aria-modal entirely; none of the hand-rolled ones wire Esc-to-close that FormModal provides). Several "sync server data into form state via useEffect" effects exist (InvoiceDetail, OfferDetail, CompanySettings, Settings); these use a one-shot ref/flag guard and are an accepted-but-not-ideal pattern per the React "You Might Not Need an Effect" guidance, so they are reported as low severity. Per React docs I pulled: avoid adjusting/resetting state on prop change in an Effect (prefer key-reset or compute during render), and data-fetching effects need an ignore/abort cleanup to avoid stale overwrites. + +**css** — The admin CSS is a single global stylesheet assembled from 19 per-feature files, all imported in AdminApp.tsx (so every selector shares one global namespace — the file split is purely organizational, not scoped). Theming is well-architected and consistent: a central variables.css drives a token system via [data-theme="dark"]/[data-theme="light"] (no prefers-color-scheme mixing), and the large majority of color usage correctly references CSS variables. Genuine issues are: (1) one undefined variable (--danger-color) used in base.css; (2) an inconsistent class-naming scheme — most files use either admin-* or a feature prefix (plan-*, dash-*, fm-*, attendance-*, offers-*), but warehouse uses admin-warehouse-*, invoices mixes admin-badge-*/invoice-*/received-*, and a block of leave badges in components.css drops the admin- prefix entirely; (3) heavy duplication of the badge color recipe (background: color-mix(... 15%) + matching text color) across leave/order/project/invoice/attendance badges, with two different recipes (--success-soft vs color-mix --success 15%) used for the same semantic color; and (4) several defined-but-unused tokens plus an empty responsive.css stub and a double-import of plan.css. Hardcoded hex outside variables.css is mostly legitimate (#fff text on accent backgrounds, feature-scoped local theme token blocks in plan.css/layout.css), not a real problem. + +**naming** — Naming in this codebase is broadly consistent and mostly intentional. Backend `src/` is overwhelmingly kebab-case for files (94 kebab vs 5 camelCase), service functions follow a clean `list*` (collections) / `get*` (single) / `create|update|delete*` (mutations) scheme with no fetch/load mixing, route verbs map cleanly to fastify methods, and Czech appears only in user-facing string literals (a documented, intentional choice — not an identifier problem). The frontend leans on its own consistent conventions: `handle*` event handlers (148 vs 8 `on*`, where the `on*` cases are local DOM listeners — idiomatic), `is*/has*` for boolean props/fields, and `*Options` for TanStack query factories (55 of them). The real, genuine deviations are a small cluster of files added during the "plan" feature work that broke the surrounding kebab-case convention (`planCategory.schema.ts`, `planCategory.service.ts`, `planAuditDescription.ts` and their tests), and the `plan.ts` query module that uses `gridQuery`/`usersQuery` instead of the established `*Options` suffix. The `.service.ts` suffix is applied to 10 of 20 service files, but this tracks a defensible entity-CRUD-vs-infra split rather than randomness. These are low-to-medium consistency gaps, not correctness issues; most fixes are renames with cross-file import ripples, so few qualify as safe-auto. + +**deadcode** — The codebase is generally well-factored (date helpers in src/utils/date.ts and frontend formatters are correctly centralized and reused). The real dead-code/duplication problems are concentrated in two areas: (1) the three PDF route files (invoices-pdf.ts, orders-pdf.ts, offers-pdf.ts) each redefine the same set of HTML/number helpers (escapeHtml, formatNum, formatCurrency, formatDate, cleanQuillHtml) plus an identical JSDOM+DOMPurify bootstrap — and the copies have already diverged (one uses a regular space as a thousands separator where the others use a non-breaking space); and (2) a handful of genuinely unused exports and one orphan component file. escapeHtml alone is independently defined six times across server and frontend. There are no leftover debug console.logs in the conventional sense — service-layer console.* calls are the established (consistent) logging pattern here since services have no Fastify logger, and the one documented console.warn in plan.service.ts is intentional. No commented-out code blocks of note were found. + +**i18n** — Localization is overwhelmingly correct and intentional: all React UI labels, placeholders, titles, button text, alert/toast messages, and the large majority of backend error strings are Czech with correct gender agreement (nenalezen/nenalezena/nenalezeno), while code identifiers, enum values, internal sentinels (e.g. EMAIL_EXISTS), env-validation throws, console.error logs, and dev-only React invariant throws ("useAuth must be used within...") are English — all intentional and not flagged. The genuine gaps are narrow: (1) a handful of English user-facing API error strings that leak to the client ("Missing attendance_id"/"Missing id" in attendance.ts, and "Unauthorized"/"Request failed (n)"/"Invalid JSON response" thrown in the frontend query layer that ProjectFileManager renders verbatim); (2) the parseBody helper surfaces raw Zod messages, so any validator lacking a custom Czech message leaks Zod's default English (enums, .max() length caps, TOTP token .min(1)); and (3) inconsistent Czech phrasing for "not found" — the compact "nenalezen*" form (92 uses) vs the verbose "nebyl/nebyla nalezen*" form (16 uses), sometimes for the identical entity ("Projekt nenalezen" vs "Projekt nebyl nalezen"). No mixed tone, no English UI chrome. + +**tests part2b** — remaining two low findings + +**config-structure** — The build/config layout is coherent: a project-references tsconfig.json splits server (CJS, ES2022, excludes src/admin) from app (ES2020, bundler resolution), with matching vite/vitest configs and env validation in src/config/env.ts. The main structural problem is the absence of a shared constants/types module spanning server and src/admin: domain label maps (entity-type, action, leave-type) and the canonical EntityType value list are duplicated and have DRIFTED. Most seriously, the frontend audit-log label/filter maps use PHP-legacy entity-type keys (offers_quotation, invoices_invoice, projects_project, trips, vehicles) that no longer match what the TS server writes (quotation, invoice, project, trip, vehicle) — a functional bug caused by the duplication. Secondary issues: a configured-but-unused @/* path alias (also absent from vite resolve), a package.json db:push script contradicting the CLAUDE.md golden rule, and .env.example drift vs config/env.ts. The one piece of genuine cross-boundary sharing (czech-holidays imported by src/admin from src/utils) is inconsistent with how everything else is duplicated and is structurally fragile. + +## Findings + +### 1. [HIGH/needs-judgment] Frontend audit-type label/filter maps use legacy keys that don't match server-written entity_type values +- **dimension:** config-structure +- **where:** src/admin/pages/AuditLog.tsx:47-66; src/admin/utils/dashboardHelpers.ts:21-40; src/types/index.ts:121-149; src/services/audit.ts:30,43; src/routes/admin/audit-log.ts:34; src/admin/lib/queries/auditLog.ts:18 +- **problem:** The server's canonical EntityType union (src/types/index.ts) and every logAudit() call write values like 'invoice', 'customer', 'quotation', 'order', 'project', 'trip', 'vehicle' (e.g. invoices.ts:134 entityType:'invoice', vehicles.ts:68 'vehicle', quotations.ts:80 'quotation'). But the frontend ENTITY_TYPE_LABELS maps (duplicated identically in AuditLog.tsx and dashboardHelpers.ts) key on PHP-legacy values: offers_quotation, offers_customer, orders_order, invoices_invoice, projects_project, trips, vehicles. Two consequences: (1) the audit log UI falls back to showing raw entity_type strings for most entities because the lookup misses; (2) ENTITY_OPTIONS (derived from this map) feeds the entity-type filter dropdown, which sends e.g. entity_type=invoices_invoice to audit-log.ts:34 where it is exact-matched (where.entity_type = String(...)), so filtering by Faktura/Objednavka/Projekt/Jizda/Vozidlo returns zero rows. +- **fix:** Make EntityType a single runtime source of truth (an `as const` array exported from a module importable by both server and src/admin) and derive both the TS union and the label maps from it, or at minimum fix the frontend keys to match the server values (invoice, customer, quotation, order, project, trip, vehicle) and de-duplicate the two copies. Add a small test asserting every EntityType value has a label. + +### 2. [HIGH/needs-judgment] NAS file manager swallows real filesystem failures with empty catch blocks and zero logging +- **dimension:** logging +- **where:** src/services/nas-file-manager.ts:118; src/services/nas-file-manager.ts:139; src/services/nas-file-manager.ts:153; src/services/nas-file-manager.ts:176; src/services/nas-file-manager.ts:426; src/services/nas-file-manager.ts:531; src/services/nas-file-manager.ts:679 +- **problem:** nas-file-manager.ts has ~25 catch blocks and not a single logging call (Grep for console./.log./logger returns no matches). While many are legitimate existence/control-flow checks where the error is expected (e.g. lines 471-473, 525-527, 666-668), several swallow genuine operation failures: deleteProjectFolder (118), createProjectFolder (139), delete (426) returning 'Nepodařilo se smazat...', createFolder mkdir (531) returning 'Nepodařilo se vytvořit složku', and countItems (679). The underlying OS error (EACCES, ENOSPC, network share down, etc.) is discarded, so when a NAS operation fails in production there is nothing in the logs to diagnose it. This directly violates the CLAUDE.md rule: 'Never silently swallow errors. Even if a failure is non-fatal, log it.' +- **fix:** In the catch blocks that represent actual operation failures (delete, mkdir, rename move, write, copy), capture the error and log it via the Fastify logger before returning the generic Czech message. Since these are service methods without request context, either accept an optional logger/request param or have the calling route log the returned error with the original cause. At minimum, distinguish 'expected ENOENT existence check' catches (which can stay silent) from 'operation should have succeeded' catches (which must log). + +### 3. [HIGH/needs-judgment] NAS financials manager swallows write/read/delete/mkdir errors without logging +- **dimension:** logging +- **where:** src/services/nas-financials-manager.ts:135; src/services/nas-financials-manager.ts:151; src/services/nas-financials-manager.ts:163; src/services/nas-financials-manager.ts:184 +- **problem:** saveReceivedInvoice (135) catches a writeFileSync failure and returns the error string to the caller but never logs it server-side. readReceivedInvoice (151-153), deleteReceivedInvoice (163-165) and ensureDir (184-186) use fully empty 'catch { return null/false }' blocks that discard the filesystem error entirely. A failed received-invoice save/read/delete on the network share leaves no server-side trace, again violating the 'never silently swallow' rule. +- **fix:** Log the caught error (with the resolved path) before returning null/false/error-string. As with nas-file-manager, thread the Fastify logger in or log the returned error at the route layer. The empty 'catch {}' forms at 151/163/184 are the worst offenders — they should at least preserve the error variable and log it. + +### 4. [HIGH/needs-judgment] plan.ts admin check reads non-existent authData.role field — admins cannot see other users' plan rows +- **dimension:** routes +- **where:** src/routes/admin/plan.ts:43-45; src/routes/admin/plan.ts:120; src/routes/admin/plan.ts:142; src/services/plan.service.ts:780; src/services/plan.service.ts:799; src/types/index.ts:51 +- **problem:** isAdminLike() does `return authData?.role === 'admin'`, but the AuthData interface (src/types/index.ts:51) has `roleName`, not `role`. Every other consumer in the codebase uses `roleName` (middleware/auth.ts:47,69; users.ts:109; services/auth.ts). So `authData.role` is always undefined and isAdminLike() always returns false. It is passed as the `isAdmin` argument to listEntries()/listOverrides() (plan.ts:120,142), where plan.service.ts:780/799 do `effectiveUserId = isAdmin ? query.user_id : actorUserId`. Net effect: GET /plan/entries and GET /plan/overrides silently ignore the user_id query param for everyone, including admins, so an admin can never read another employee's raw plan/override rows. (Security direction is safe — it over-restricts rather than leaking — but the feature is broken for admins.) +- **fix:** Change isAdminLike to use the documented role/permission model: either `authData?.roleName === 'admin'` to match middleware/auth.ts, or better, check the relevant permission (e.g. authData.permissions.includes('attendance.manage')) consistent with how trips.ts:22 and attendance.ts:208 distinguish admin vs self. Also drop the `any` type on the parameter and type it as AuthData. + +### 5. [HIGH/needs-judgment] `any`-typed param hides admin-scoping bug: isAdminLike reads nonexistent `authData.role` +- **dimension:** ts-safety +- **where:** src/routes/admin/plan.ts:43; src/routes/admin/plan.ts:44; src/routes/admin/plan.ts:120; src/routes/admin/plan.ts:142; src/types/index.ts:44; src/services/plan.service.ts:780; src/services/plan.service.ts:799 +- **problem:** `isAdminLike(authData: any)` returns `authData?.role === "admin"`. The `AuthData` type (types/index.ts:44) has NO `role` field — the admin role is on `roleName` (used correctly everywhere else: auth.ts:47/69, users.ts:109). Because the parameter is typed `any`, the compiler cannot flag that `.role` is always `undefined`, so `isAdminLike` always returns `false`. It is passed as the `isAdmin` arg to `listEntries`/`listOverrides`, where `effectiveUserId = isAdmin ? query.user_id : actorUserId`. Result: an admin querying another user's plan is silently scoped to their own rows; the `user_id` query param is ignored for everyone. The `any` is the direct cause the type system did not catch the `role` vs `roleName` typo. +- **fix:** Type the parameter as `AuthData | null | undefined` (or reuse the existing `roleName === "admin"` convention) and change the comparison to `authData?.roleName === "admin"`. This both fixes the bug and lets the compiler verify the field exists. Add/confirm a list-scoping test for the cross-user admin case. + +### 6. [MEDIUM/needs-judgment] Domain label maps duplicated verbatim across frontend files (and diverging) +- **dimension:** config-structure +- **where:** src/admin/utils/dashboardHelpers.ts:1-47; src/admin/pages/AuditLog.tsx:17-66; src/admin/utils/attendanceHelpers.ts:80-97; src/admin/components/ShiftFormModal.tsx:338-340 +- **problem:** ENTITY_TYPE_LABELS and ACTION_LABELS exist as near-identical copies in dashboardHelpers.ts and AuditLog.tsx, and have already diverged: ACTION_LABELS.create is 'Vytvoreni' in AuditLog but 'Vytvoril' in dashboardHelpers, and AuditLog's copy has extra entries (view, activate, deactivate, password_change, ...). Separately, leave-type labels (vacation->'Dovolena', sick->'Nemoc', unpaid->'Neplacene volno') are repeated in dashboardHelpers.ts:2-4, attendanceHelpers.ts:83-85, and inline