Maximilian Stubhan
79f0e19bf8
Data model - employee_assignments records org placement over time (valid_from/valid_to), written by a trigger on `employees` rather than inside each RPC: ~70 `update employees` statements spread over fifteen migrations mean per-call bookkeeping would miss paths today and again with every future RPC. A partial unique index enforces the one-open-interval invariant the trigger relies on when closing the current row. - The Organigramm gains a Stichtag (default today). Membership comes from entry/exit/karenz, past placement from the new history, future placement projected from pending_org_changes. Placements predating the migration are backfilled with today's values and flagged as such in the UI, since employee_history only ever stored free text and cannot be reconstructed. Correctness - Reports and exports silently truncated at PostgREST's 1000-row cap (db.max_rows); employee_history is already past it at ~800 staff. Every whole-table read now pages explicitly. - XLSX date cells were a day early: ExcelJS converts a Date to an Excel serial straight off getTime(), so a Date built at local midnight lands on the previous day's serial in any positive-offset zone. - Date handling is pinned to Europe/Vienna throughout, and date-only strings are formatted without a Date round-trip. The dashboard's YTD window was built by round-tripping a local Date through toISOString(), which shifted it a day early and dropped 31 December entirely. - Export routes parsed measure/group/split/eventType with unchecked `as` casts, so an unknown value reached column headers as `undefined` and the Content-Disposition filename. Parsed against the label maps now, with the filename slugged as a backstop. - toXlsx keyed columns by header text, silently dropping the second of any two columns sharing a name — split columns take their header from data. - The org chart tree walks had no cycle guard; nothing in the schema forbids a manager_id cycle, and one would hang the tab rather than misreport. - The login page reflected ?error= verbatim, letting anyone put arbitrary text on the real sign-in screen; messages are looked up by code now. - React Flow needs elementsSelectable on, or it sets pointer-events:none on the whole node and the expand control stops responding. UI - Mobile: the shell was unusable below lg — a fixed 236px margin pushed content off-screen with no mobile navigation at all. The sidebar is now a drawer, dvh replaces vh, safe-area insets are honoured, inputs are 16px so iOS stops zooming on focus, and form grids stack. - Org chart nodes redesigned: per-kind accent stripes and icons, vacant roles called out, expand control moved to the bottom edge carrying the child count. - Pagination is windowed; it previously rendered one link per page (54 for the employee list, unbounded for the audit log). - Positions page reduced to open positions with a single "Besetzen" action. - The employee Organisation tab links into the org chart focused on that person, reusing the chart's existing search-match highlighting. Also included, uncommitted until now - Dependants, HR notes, academic titles, split address fields, position validity and role/employment fields, with their migrations and UI. - Docker/compose deployment setup, data-model and security-review docs.
114 lines
5.5 KiB
Markdown
114 lines
5.5 KiB
Markdown
# Security Review
|
|
|
|
Ergebnis eines gezielten Greps über das gesamte Repository (ohne
|
|
`node_modules`) nach neun sicherheitsrelevanten Mustern, mit Bewertung im
|
|
jeweiligen Kontext. Stand: 2026-07-24.
|
|
|
|
## `SUPABASE_SERVICE_ROLE_KEY`
|
|
|
|
Referenziert in vier Dateien, alle server-seitig / lokal:
|
|
|
|
- `lib/supabase/admin.ts` — die einzige App-Laufzeit-Verwendung, hinter
|
|
`import "server-only"`. Jetzt mit expliziter Fehlermeldung bei fehlendem
|
|
Wert statt eines `!`-Non-null-Assertions (siehe Änderungen).
|
|
- `supabase/seed.ts`, `tests/integration/helpers.ts` — Node-Skripte
|
|
außerhalb des Next.js-Bundles (Seeding bzw. Test-Setup), lesen den Wert
|
|
nur aus `process.env`. Unkritisch.
|
|
- `.scratch_make_test_hr.mjs` — lokales, nicht eingechecktes
|
|
Hilfsskript (liest den Key ebenfalls nur aus `process.env`, kein
|
|
Hardcoding). War bisher weder committet noch von `.gitignore` erfasst;
|
|
**behoben** — `.gitignore` schließt `.scratch_*` jetzt explizit aus, damit
|
|
ein künftiges `git add -A` es nicht versehentlich eincheckt.
|
|
|
|
Keine Fundstelle exponiert den Key im Browser-Bundle oder in einer
|
|
API-Response.
|
|
|
|
## `service_role` (Postgres-Rolle)
|
|
|
|
9 Treffer, ausschließlich in `supabase/migrations/*.sql` — Standard-Supabase-
|
|
Muster:
|
|
|
|
- `20260714120500_default_grants.sql`: explizite `grant ... to anon,
|
|
authenticated, service_role` (nötig, weil RLS Objekt-Rechte nur
|
|
einschränkt, nicht ersetzt — ohne dieses Grant schlägt jede Query auch
|
|
mit korrekter Policy mit "permission denied" fehl).
|
|
- `20260714120200_effective_dating_rpcs.sql` / `20260714120600_...`:
|
|
`revoke execute ... from public, anon, authenticated; grant execute ...
|
|
to service_role` für `apply_due_pending_changes()` — das ist die
|
|
**korrekte** Absicherung: die Funktion darf nur vom Cron-Job (über den
|
|
Service-Role-Client) aufgerufen werden, nicht von einer eingeloggten
|
|
HR-Person, sonst könnte diese noch nicht fällige Änderungen vorzeitig
|
|
erzwingen.
|
|
|
|
Keine problematische Fundstelle.
|
|
|
|
## `localStorage`, `sessionStorage`, `document.cookie`
|
|
|
|
Keine Treffer im gesamten App-Code. Sessions laufen ausschließlich über den
|
|
von `@supabase/ssr` verwalteten Cookie-Adapter (`lib/supabase/server.ts`,
|
|
`proxy.ts`), nicht über direkten Browser-Storage-Zugriff. Kein Risiko einer
|
|
Token-Exponierung über clientseitigen Storage.
|
|
|
|
## `dangerouslySetInnerHTML`, `innerHTML`
|
|
|
|
Keine Treffer. Keine rohe HTML-Injection-Fläche im Code.
|
|
|
|
## `Authorization`
|
|
|
|
Einzige Fundstelle: `tests/unit/security.test.ts` (prüft den Cron-Route-
|
|
Guard). Die Route selbst liest den Header über
|
|
`request.headers.get("authorization")` (Kleinschreibung) — HTTP-Header sind
|
|
case-insensitiv und `Headers.get()` behandelt sie entsprechend, das ist
|
|
korrekt und wird bereits durch drei Tests abgedeckt (fehlender Header,
|
|
falscher Wert, nicht konfiguriertes `CRON_SECRET`).
|
|
|
|
## `audit_logs` / `planned_changes` (aus der Aufgabenstellung)
|
|
|
|
Keine Treffer unter diesen exakten Namen — das reale Schema heißt
|
|
`audit_log` (Singular) bzw. `pending_org_changes`. Siehe
|
|
[`docs/data-model.md`](data-model.md) für die vollständige Zuordnung.
|
|
|
|
**Audit-Log-Abdeckung geprüft:** Jede mutierende Server Action (`actions/
|
|
employees.ts`, `actions/positions.ts`, `actions/reorg.ts`) ruft ausschließlich
|
|
`supabase.rpc(...)` auf — keine einzige schreibt direkt per
|
|
`.from(...).insert()/.update()/.delete()` auf `employees`, `positions`,
|
|
`employee_notes`, `employee_dependents` oder `profiles`. Jede der
|
|
dahinterliegenden SQL-Funktionen (`hire_employee`, `transfer_employee`,
|
|
`promote_employee`, `start_karenz`, `record_karenz_return`,
|
|
`change_employee_data`, `apply_reorg`, `undo_reorg`,
|
|
`staff_position_internally`, `delete_position`, `add_employee_dependent`,
|
|
`delete_employee_dependent`, `add_employee_note`,
|
|
`complete_employee_note`) schreibt ihren `audit_log`-Eintrag in derselben
|
|
Transaktion wie die eigentliche Datenänderung. Für die aktuell existierenden
|
|
Mutationspfade ist damit lückenlos sichergestellt, dass keine Änderung ohne
|
|
Audit-Eintrag möglich ist — ein fehlgeschlagener Audit-Insert lässt die
|
|
gesamte Transaktion fehlschlagen.
|
|
|
|
Ausnahme (bewusst, kein Gap): `apply_due_pending_changes()` (vom Cron-Job
|
|
aufgerufen) schreibt beim tatsächlichen Anwenden einer fälligen Änderung
|
|
keinen zusätzlichen `audit_log`-Eintrag — der Audit-Eintrag für diese
|
|
Änderung wurde bereits zum Zeitpunkt der Anforderung geschrieben (von
|
|
`transfer_employee`/`start_karenz`/etc.), datiert auf das Wirksamkeitsdatum.
|
|
Ein zweiter Eintrag beim tatsächlichen Anwenden würde denselben
|
|
Geschäftsvorfall doppelt loggen.
|
|
|
|
## Warum hier kein neues `lib/audit/audit-log.ts` entstanden ist
|
|
|
|
Eine App-seitige `writeAuditLog()`-Hilfsfunktion wäre eine zweite,
|
|
nicht-transaktionale Logging-Quelle neben der bestehenden — sie könnte
|
|
fehlschlagen, nachdem die eigentliche Mutation bereits committet wurde, und
|
|
so eine Änderung ohne Audit-Spur hinterlassen. Die bestehende Lösung
|
|
(Audit-Insert in derselben SQL-Funktion/Transaktion) ist strenger. Ein
|
|
App-seitiger Helfer wäre daher eine Verschlechterung, kein Fix.
|
|
|
|
## Offene Punkte (siehe README → "Known TODOs")
|
|
|
|
- Kein Content-Security-Policy-Header (bewusst zurückgestellt, siehe
|
|
Kommentar in `next.config.ts` — Skript-/Style-/Connect-Quellen noch nicht
|
|
vollständig inventarisiert).
|
|
- `.scratch_shots/` enthielt PNG-Screenshots einer Testsitzung mit
|
|
Beispiel-Notiztext ("vertrauliches Gespräch zur Verifikation" — Testdaten,
|
|
keine echten Personendaten identifiziert). Ordner ist jetzt über
|
|
`.gitignore` ausgeschlossen; Inhalt selbst wurde nicht gelöscht (siehe
|
|
Hinweis unten).
|