Files
app/docs/superpowers/plans/2026-06-10-audit-high-fix-pass.md
BOHA 1826fc7976 fix: audit fix pass #1 — all 19 verified HIGH findings + critical dep cleanup
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>
2026-06-10 09:59:47 +02:00

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.