Fixes every CRITICAL/HIGH finding from the 2026-06-09 full-codebase audit
(REVIEW_FINDINGS.md); each fix went through independent spec + code-quality
review. Plan and per-task log: docs/superpowers/plans/2026-06-10-audit-high-fix-pass.md
- attendance: schemas accept the combined local datetimes the forms/service
use (new dateTimeString helpers in schemas/common.ts), breaks persist on
create, AttendanceCreate submit rebuilt — every submit 400'd since 519edce
- 2fa: backup codes wired to /totp/backup-verify (+ remember-me parity),
enrollment QR generated locally via qrcode (CSP-blocked external service
also leaked the secret), dashboard shows per-user enrollment, not policy
- invoices/orders: per-line VAT survives re-saves (numberOr 0-respecting
coercion in formatters.ts), billing_text persists on update, issued-order
status transitions update UI gates
- trips: real pagination on all 3 pages, GET /trips/stats server aggregate
(shared buildTripsWhere + legacy distance coalesce), vehicle_id applies on
PUT with both-vehicle odometer recompute, print rebuilt (sync window.open,
escaped template, server totals)
- orders api: attachment_data PDF blob excluded from all non-binary reads
- warehouse: unit field is a Select over UnitEnum, receipt attachments
downloadable via new authenticated GET route
- downloads: shared RFC 5987 contentDisposition helper — Czech filenames no
longer 500 (warehouse, received-invoices, orders endpoints)
- misc: block-env hook actually blocks (exit 2 + stderr), project create
works with empty dates, NaN filter guards on trips endpoints
- deps: remove unused concurrently (clears both critical advisories), pin
@hono/node-server >=1.19.13 via overrides (clears the 3 moderates without
the Prisma 6 downgrade), drop deprecated @types stubs
Gates: tsc -b clean - vitest 30 files / 342 tests (31 new) - eslint 0 errors
- build OK
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
189 lines
11 KiB
Markdown
189 lines
11 KiB
Markdown
# 2026-06-10 — Audit fix pass #1: all verified CRITICAL/HIGH findings
|
|
|
|
Source: `REVIEW_FINDINGS.md` (2026-06-09 full audit, 304 files). Scope of this pass:
|
|
the 19 verified per-file **HIGH** findings + the project-level **CRITICAL** dependency
|
|
finding. The two project-level HIGH _structural refactors_ (service-layer extraction for
|
|
6 route files; PDF template dedup) are explicitly **out of scope** — they are
|
|
multi-thousand-line refactors that deserve their own pass.
|
|
|
|
Execution: subagent-driven development — one implementer per task (sequential, shared
|
|
test DB forbids parallel test runs), spec-compliance review then code-quality review
|
|
after each. No commits until the user asks. Final full gates:
|
|
`npx tsc -b --noEmit` · `npx vitest run` · `npm run lint` · `npm run build`.
|
|
|
|
---
|
|
|
|
## Task H — Dependencies (project-level CRITICAL) — done inline by controller
|
|
|
|
- Remove `concurrently` from devDependencies (unused; carries both critical advisories
|
|
GHSA-w7jw-789q-3m8p / shell-quote CVSS 8.1).
|
|
- Add `"overrides": { "@hono/node-server": "^1.19.13" }` (clears the 3 moderate
|
|
advisories from the prisma → @prisma/dev chain; do NOT take npm's Prisma 6 downgrade).
|
|
- Remove deprecated stub types `@types/dompurify`, `@types/bcryptjs`; move `@types/jsdom`
|
|
to devDependencies.
|
|
- Add `qrcode` + `@types/qrcode` (needed by Task B to replace the CSP-blocked external
|
|
QR service).
|
|
- `npm install`, verify `npm audit` (criticals+moderates gone), typecheck.
|
|
|
|
## Task A — Attendance create/edit broken since 519edce (top-5 #1)
|
|
|
|
Findings (both verified HIGH):
|
|
|
|
- `src/admin/pages/AttendanceCreate.tsx:80-99` — handleSubmit posts the raw form whose
|
|
shape the API does not accept: `arrival_date`/`break_start_date`/`break_start_time`/
|
|
`break_end_*`/`departure_date` are not in CreateAttendanceSchema (silently stripped —
|
|
breaks and overnight dates never save); times are never combined with dates (unlike
|
|
useAttendanceAdmin's combineDatetime), so a validated "HH:MM" would reach
|
|
createAttendance which does `new Date('08:00')` → Invalid Date → 500; and since commit
|
|
519edce the always-sent empty-string time fields (`arrival_time: ""`) fail timeString
|
|
validation, so EVERY submit from this page — including leave records — returns 400.
|
|
- `src/admin/pages/AttendanceAdmin.tsx:407-442` (logic in `useAttendanceAdmin.ts`) —
|
|
create/edit modal submits send combined `YYYY-MM-DDTHH:MM:00` datetimes for
|
|
arrival/departure/break, but since 519edce CreateAttendanceSchema/UpdateAttendanceSchema
|
|
validate them with `timeString` (HH:MM only) — every create/edit containing a time is
|
|
rejected 400 "Čas musí být ve formátu HH:MM" (and the service itself expects full
|
|
datetimes via `new Date(...)`); additionally `break_start`/`break_end` are absent from
|
|
CreateAttendanceSchema, so breaks are silently stripped on create.
|
|
|
|
Direction: first `git show 519edce` to understand the intent; the schema contradicts
|
|
both client and service, so the fix is to make the schemas validate the combined
|
|
local-datetime wire format the client sends and the service consumes (lenient helper in
|
|
`src/schemas/common.ts` per repo convention), add `break_start`/`break_end` to the create
|
|
schema, and rewrite AttendanceCreate.tsx's submit to combine date+time and omit empty
|
|
optionals. TDD: route-level regression tests (work record with times+breaks, overnight,
|
|
leave record without times, update) in `src/__tests__/`.
|
|
|
|
## Task B — 2FA: backup-code login, enrollment QR, dashboard status (top-5 #2)
|
|
|
|
Findings (all verified HIGH):
|
|
|
|
- `src/admin/context/AuthContext.tsx:263-272` — backup-code login broken: verify2FA
|
|
always POSTs to /login/totp with the code as totp_code plus an isBackup flag the server
|
|
ignores; TotpVerifySchema requires exactly 6 digits so 8-char backup codes always 400.
|
|
The real endpoint POST /api/admin/totp/backup-verify (expects backup_code) is never
|
|
called anywhere in the frontend — lockout for any user who lost their authenticator.
|
|
- `src/admin/components/dashboard/DashProfile.tsx:556` — QR `<img>` from api.qrserver.com
|
|
is blocked by the production CSP (img-src 'self' data: blob: + osm tiles), so prod 2FA
|
|
enrollment shows a broken image. (Also a privacy bug: the TOTP secret is sent to a
|
|
third-party URL.) Fix: generate the QR locally with the `qrcode` package
|
|
(`QRCode.toDataURL(otpauthUrl)`) — data: is already CSP-allowed.
|
|
- `src/admin/pages/Dashboard.tsx:89-91` — totpEnabled derived from require2FAOptions()
|
|
which returns the COMPANY-WIDE require_2fa policy flag, not the user's enrollment; the
|
|
per-user endpoint (totp.ts:190-197) is never queried. Wrong "2FA Aktivní/Neaktivní" and
|
|
wrong button shown.
|
|
|
|
Direction: read `src/routes/admin/totp.ts` first; wire Login/AuthContext so a backup code
|
|
goes to backup-verify and completes the login-token flow. If the server endpoint doesn't
|
|
fit the loginToken flow, extend it minimally (single-use code consumption + logAudit
|
|
preserved). Server test for backup-verify login path if server is touched.
|
|
|
|
## Task C — Invoice/issued-order edit integrity (top-5 #3)
|
|
|
|
Findings (all verified HIGH):
|
|
|
|
- `src/admin/pages/InvoiceDetail.tsx:728` — edit hydration overwrites each item's
|
|
persisted per-line VAT with the invoice-level rate (`vat_rate: Number(inv.vat_rate) || 21`)
|
|
though invoice_items.vat_rate is a real per-item column — re-saving a mixed-rate invoice
|
|
silently corrupts per-line VAT in the DB; `|| 21` also turns legitimate 0% into 21%
|
|
(same falsy-zero at line 689).
|
|
- `src/admin/pages/InvoiceDetail.tsx:1891-1902` — editing "Text fakturace (na PDF)" is
|
|
silently lost: payload includes billing_text but UpdateInvoiceSchema
|
|
(src/schemas/invoices.schema.ts:46-67) has no billing_text field, so Zod strips it
|
|
(updateInvoice supports it via strFields). Add the field to the schema + server test.
|
|
- `src/admin/pages/IssuedOrderDetail.tsx:606-611` — hydration falsy-fallbacks:
|
|
`quantity: Number(it.quantity) || 1` turns saved 0 into 1; `vat_rate: Number(it.vat_rate)
|
|
|| Number(d.vat_rate) || 21` turns a saved 0% line into 21% — displayed wrong and
|
|
silently persisted on next save.
|
|
- `src/admin/pages/IssuedOrderDetail.tsx:826-839` — handleStatusChange never mirrors the
|
|
new status into form.status, so StatusChip, the `editable` flag and the delete-button
|
|
gate use the stale status after every transition.
|
|
|
|
Use Number.isFinite-style coercion that respects 0 (e.g. `const n = Number(x);
|
|
Number.isFinite(n) ? n : fallback`).
|
|
|
|
## Task D — Trips: truncated lists/totals + dropped vehicle edit (top-5 #4a)
|
|
|
|
Findings (all verified HIGH):
|
|
|
|
- `src/admin/lib/queries/trips.ts:52-63` — tripListOptions consumes the paginated
|
|
endpoint via plain jsonQuery, discarding pagination meta; Trips.tsx uses default limit
|
|
25 and TripsAdmin perPage:100 (server hard cap) — both silently truncate with no
|
|
pagination controls.
|
|
- `src/admin/lib/queries/trips.ts:95-103` — tripHistoryOptions sends no page/per_page →
|
|
25 rows; TripsHistory.tsx computes monthly km/business totals from the truncated rows.
|
|
- `src/admin/pages/Trips.tsx:324-336` — stat cards computed from the 25-row page;
|
|
header labels stats "current month" though the query has no month filter.
|
|
- `src/admin/pages/TripsAdmin.tsx:295,665-676` — edit modal sends vehicle_id but
|
|
UpdateTripSchema (src/schemas/trips.schema.ts:23-31) has no vehicle_id and the PUT
|
|
handler never applies it — vehicle change silently dropped. Add to schema
|
|
(nullableIntIdFromForm) + apply in route + audit + server test.
|
|
- `src/admin/pages/TripsHistory.tsx:98-104,121-128` — same 25-row truncation for the
|
|
month view and its stat cards.
|
|
|
|
Direction for totals: prefer a server-side aggregate (the repo already has per-currency
|
|
totals endpoints for invoices/orders as the pattern) over fetching all pages client-side;
|
|
lists get real pagination using the existing `usePaginatedQuery`/`Pagination` kit pieces
|
|
used elsewhere. Keep the month-filter semantics of TripsHistory correct (totals must
|
|
cover the WHOLE month, not one page).
|
|
|
|
## Task E — orders.service.ts: PDF blob in every response (top-5 #4b)
|
|
|
|
Finding (verified HIGH): `src/services/orders.service.ts:154-176,196-216,228-263,570,683`
|
|
— attachment_data (full PDF blob, Bytes) is fetched on every query with no select/omit
|
|
and spread into API responses by enrichOrder/getOrder; getOrderTotals loads every blob
|
|
for the whole filtered set just to sum totals; updateOrder/deleteOrder pre-reads pull the
|
|
blob to check status/number. A dedicated /:id/attachment endpoint already exists.
|
|
Exclude the blob from list/detail/totals/pre-reads (keep has_attachment-style boolean if
|
|
the FE needs it — check FE usage), keep the attachment endpoint working, extend
|
|
`src/__tests__/orders-list.test.ts` to assert attachment_data is absent.
|
|
|
|
## Task F — Warehouse: unit enum 400s + dead attachment download
|
|
|
|
Findings (both verified HIGH):
|
|
|
|
- `src/admin/pages/WarehouseItemDetail.tsx:451-462` — "Jednotka" is free-text but the
|
|
server requires UnitEnum ("ks","m","kg","bal","sada","m2","m3","l") — any other value
|
|
400s create/update. Make it a Select over the enum values (single source: mirror the
|
|
server enum list).
|
|
- `src/admin/pages/WarehouseReceiptDetail.tsx:251-259` — attachment link targets
|
|
GET /warehouse/receipts/:id/attachments/:attachmentId which does not exist (only POST
|
|
upload + DELETE are registered in warehouse.ts), and a plain <a href> sends no
|
|
Authorization header anyway. Add the GET download route (requirePermission, correct
|
|
content-type/disposition) + authenticated blob download in the UI (fetch via apiFetch →
|
|
object URL, like other binary downloads in the repo). Server test for the new route.
|
|
|
|
## Task G — Small fixes
|
|
|
|
- `.claude/hooks/block-env.js:10-11` (verified HIGH) — block decision never takes
|
|
effect: prints {decision:'block'} JSON to stdout then exits 1; Claude Code only parses
|
|
stdout JSON on exit 0 and only blocks on exit 2 (reason on stderr). Fix: exit 2 with the
|
|
reason on stderr.
|
|
- `src/admin/pages/Projects.tsx:153-154` (verified HIGH) — create modal submits
|
|
start_date/end_date initialized to "" which isoDateString.nullish() rejects → creating
|
|
a project without filling BOTH dates always 400s. Send null/omit when empty.
|
|
|
|
---
|
|
|
|
## Status
|
|
|
|
- [x] H — dependencies (controller, inline) — criticals+moderates cleared; only the
|
|
2 known-mitigated quill lows remain
|
|
- [x] A — attendance create/edit — dateTimeString helpers, break fields, rewritten
|
|
AttendanceCreate payload; 6 regression tests
|
|
- [x] B — 2FA — backup-verify wired (+ remember-me parity server-side), local QR via
|
|
qrcode lib, per-user totp status; 6 tests
|
|
- [x] C — invoice/issued-order edit integrity — numberOr (shared in formatters.ts),
|
|
billing_text in UpdateInvoiceSchema, status mirror; 2 tests
|
|
- [x] D — trips — paginated queries + Pagination UI on 3 pages, GET /trips/stats
|
|
(buildTripsWhere shared), vehicle_id on PUT + both-vehicle odometer recompute,
|
|
print rebuilt (sync window.open, escaped string template); 7 tests
|
|
- [x] E — orders attachment_data excluded from all non-binary reads (incl.
|
|
numbering/invoices/orders-pdf callers); 4 tests
|
|
- [x] F — warehouse unit Select + GET receipt attachment route + authenticated blob
|
|
download; RFC 5987 content-disposition helper (also fixes received-invoices +
|
|
orders Czech-filename 500); 6 tests
|
|
- [x] G — block-env hook exit 2/stderr (verified empirically), Projects empty dates
|
|
→ null
|
|
- [x] Final gates: tsc -b clean · vitest 30 files / 342 tests green · lint 0 errors ·
|
|
build OK. Whole-diff 3-lens review: see final report.
|