feat: comprehensive MVP audit — security, architecture, UX, competitive analysis

Structured assessment covering code quality, security (critical tenant isolation bypass found), architecture, UX gaps, testing coverage, deployment, and competitive positioning vs RA-MICRO/ADVOWARE/AnNoText/Actaport. Includes prioritized roadmap (P0-P3) with actionable items.
2026-03-28 02:22:07 +01:00
parent 7c7ae396f4
commit 60f1f4ef4a
1 changed files with 427 additions and 0 deletions
--- a/AUDIT.md
+++ b/AUDIT.md
@@ -0,0 +1,427 @@
 # KanzlAI-mGMT MVP Audit
 **Date:** 2026-03-28
 **Auditor:** athena (consultant)
 **Scope:** Full-stack audit of KanzlAI-mGMT — Go backend, Next.js frontend, Supabase database, deployment, security, UX, competitive positioning.
 **Codebase:** ~16,500 lines across ~60 source files, built 2026-03-25 in a single session with parallel workers.
 ---
 ## Executive Summary
 KanzlAI-mGMT is an impressive MVP built in ~2 hours. It covers the core Kanzleimanagement primitives: cases, deadlines, appointments, parties, documents, notes, dashboard, CalDAV sync, and AI-powered deadline extraction. The architecture is sound — clean separation between Go API and Next.js frontend, proper multi-tenant design with Supabase Auth, parameterized SQL throughout.
 However, the speed of construction shows. There are **critical security gaps** that must be fixed before any external user touches this. The frontend has good bones but lacks the polish and completeness a lawyer would expect. And the feature gap vs. established competitors (RA-MICRO, ADVOWARE, AnNoText, Actaport) is enormous — particularly around beA integration, billing/RVG, and document generation, which are table-stakes for German law firms.
 **Bottom line:** Fix the security issues, add error recovery and multi-tenant auth verification, then decide whether to pursue the Kanzleimanagement market (massive feature gap) or pivot back to the UPC niche (where you had a genuine competitive advantage).
 ---
 ## 1. Critical Issues (Fix Immediately)
 ### 1.1 Tenant Isolation Bypass in TenantResolver
 **File:** `backend/internal/auth/tenant_resolver.go:37-42`
 When the `X-Tenant-ID` header is provided, the TenantResolver parses it and sets it in context **without verifying the user has access to that tenant**. Any authenticated user can access any tenant's data by setting this header.
 ```go
 if header := r.Header.Get("X-Tenant-ID"); header != "" {
    parsed, err := uuid.Parse(header)
    // ... sets tenantID = parsed — NO ACCESS CHECK
 }
 ```
 Compare with `helpers.go:32-44` where `resolveTenant()` correctly verifies access via `user_tenants` — but this function is unused in the middleware path. The TenantResolver middleware is what actually runs for all scoped routes.
 **Impact:** Complete tenant data isolation breach. User A can read/modify/delete User B's cases, deadlines, appointments, documents.
 **Fix:** Add `user_tenants` lookup in TenantResolver when X-Tenant-ID is provided, same as `resolveTenant()` does.
 ### 1.2 Duplicate Tenant Resolution Logic
 **Files:** `backend/internal/auth/tenant_resolver.go` and `backend/internal/handlers/helpers.go:25-57`
 Two independent implementations of tenant resolution exist. The middleware (`TenantResolver`) is used for the scoped routes. The handler-level `resolveTenant()` function exists in helpers.go. The auth middleware in `middleware.go:39-47` also resolves a tenant into context. This triple-resolution creates confusion and the security bug above.
 **Fix:** Consolidate to a single path. Remove the handler-level `resolveTenant()` and the auth middleware's tenant resolution. Let TenantResolver be the single source of truth, but make it verify access.
 ### 1.3 CalDAV Credentials Stored in Plaintext
 **File:** `backend/internal/services/caldav_service.go:29-35`
 CalDAV username and password are stored as plain JSON in the `tenants.settings` column:
 ```go
 type CalDAVConfig struct {
    URL      string `json:"url"`
    Username string `json:"username"`
    Password string `json:"password"`
    ...
 }
 ```
 Combined with the tenant isolation bypass above, any authenticated user can read any tenant's CalDAV credentials.
 **Fix:** Encrypt CalDAV credentials at rest (e.g., using `pgcrypto` or application-level encryption). At minimum, never return the password in API responses.
 ### 1.4 No CORS Configuration
 **File:** `backend/internal/router/router.go`, `backend/cmd/server/main.go`
 There is zero CORS handling anywhere in the backend. The frontend uses Next.js rewrites to proxy `/api/` to the backend, which works in production. But:
 - If anyone accesses the backend directly (different origin), there's no CORS protection.
 - No `X-Frame-Options`, `X-Content-Type-Options`, or other security headers are set.
 **Fix:** Add CORS middleware restricting to the frontend origin. Add standard security headers.
 ### 1.5 Internal Error Messages Leaked to Clients
 **Files:** Multiple handlers (e.g., `cases.go:44`, `cases.go:73`, `appointments.go`)
 ```go
 writeError(w, http.StatusInternalServerError, err.Error())
 ```
 Internal error messages (including SQL errors, connection errors, etc.) are sent directly to the client. This leaks implementation details.
 **Fix:** Log the full error server-side, return a generic message to the client.
 ---
 ## 2. Important Gaps (Fix Before Showing to Anyone)
 ### 2.1 No Input Validation Beyond "Required Fields"
 **Files:** All handlers
 Input validation is minimal — typically just checking if required fields are empty:
 ```go
 if input.CaseNumber == "" || input.Title == "" {
    writeError(w, http.StatusBadRequest, "case_number and title are required")
 }
 ```
 Missing:
 - Length limits on text fields (could store megabytes in a title field)
 - Status value validation (accepts any string for status fields)
 - Date format validation
 - Case type validation against allowed values
 - SQL-safe string validation (although parameterized queries protect against injection)
 ### 2.2 No Pagination Defaults on Most List Endpoints
 **File:** `backend/internal/services/case_service.go:57-63`
 `CaseService.List` has sane defaults (limit=20, max=100). But other list endpoints (`appointments`, `deadlines`, `notes`, `parties`, `case_events`) have no pagination at all — they return all records for a tenant/case. As data grows, these become performance problems.
 ### 2.3 Dashboard Page is Entirely Client-Side
 **File:** `frontend/src/app/(app)/dashboard/page.tsx`
 The entire dashboard is a `"use client"` component that fetches data via API. This means:
 - No SSR benefit — the page is blank until JS loads and API responds
 - SEO doesn't matter for a SaaS app, but initial load time does
 - The skeleton is nice but adds 200-400ms of perceived latency
 For an internal tool this is acceptable, but for a commercial product it should use server components for the initial render.
 ### 2.4 Frontend Auth Uses `getSession()` Instead of `getUser()`
 **File:** `frontend/src/lib/api.ts:10-12`
 ```typescript
 const { data: { session } } = await supabase.auth.getSession();
 ```
 `getSession()` reads from local storage without server verification. If a session is expired or revoked server-side, the frontend will still try to use it until the backend rejects it. The middleware correctly uses `getUser()` (which validates server-side), but the API client does not.
 ### 2.5 Missing Error Recovery in Frontend
 Throughout the frontend, API errors are handled with basic error states, but there's no:
 - Retry logic for transient failures
 - Token refresh on 401 responses
 - Optimistic UI rollback on mutation failures
 - Offline detection
 ### 2.6 Missing `Content-Disposition` Header Sanitization
 **File:** `backend/internal/handlers/documents.go:133`
 ```go
 w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, title))
 ```
 The `title` (which comes from user input) is inserted directly into the header. A filename containing `"` or newlines could be used for response header injection.
 **Fix:** Sanitize the filename — strip or encode special characters.
 ### 2.7 No Graceful Shutdown
 **File:** `backend/cmd/server/main.go:42`
 ```go
 http.ListenAndServe(":"+cfg.Port, handler)
 ```
 No signal handling or graceful shutdown. When the process receives SIGTERM (e.g., during deployment), in-flight requests are dropped, CalDAV sync operations may be interrupted mid-write, and database connections are not cleanly closed.
 ### 2.8 Database Connection Pool — search_path is Session-Level
 **File:** `backend/internal/db/connection.go:17`
 ```go
 db.Exec("SET search_path TO kanzlai, public")
 ```
 `SET search_path` is session-level in PostgreSQL. With connection pooling (`MaxOpenConns: 25`), this SET runs once on the initial connection. If a connection is recycled or a new one opened from the pool, it may not have the kanzlai search_path. This could cause queries to silently hit the wrong schema.
 **Fix:** Use `SET LOCAL search_path` in a transaction, or set it at the database/role level, or qualify all table references with the schema name.
 ### 2.9 go.sum Missing from Dockerfile
 **File:** `backend/Dockerfile:4`
 ```dockerfile
 COPY go.mod ./
 RUN go mod download
 ```
 Only `go.mod` is copied, not `go.sum`. This means the build isn't reproducible and doesn't verify checksums. Should be `COPY go.mod go.sum ./`.
 ---
 ## 3. Architecture Assessment
 ### 3.1 What's Good
 - **Clean monorepo structure** — `backend/` and `frontend/` are clearly separated. Each has its own Dockerfile. The Makefile provides unified commands.
 - **Go backend is well-organized** — `cmd/server/`, `internal/{auth,config,db,handlers,middleware,models,router,services}` follows Go best practices.
 - **Handler/Service separation** — handlers do HTTP concerns (parse request, write response), services do business logic. This is correct.
 - **Parameterized SQL everywhere** — no string concatenation in queries. All user input goes through `$N` placeholders.
 - **Multi-tenant design** — `tenant_id` on every row, context-based tenant resolution, RLS at the database level.
 - **Smart use of Go 1.22+ routing** — method+path patterns like `GET /api/cases/{id}` eliminate the need for a third-party router.
 - **CalDAV sync is genuinely impressive** — bidirectional sync with conflict resolution, etag tracking, background polling per-tenant. This is a differentiator.
 - **Deadline calculator** — ported from youpc.org with holiday awareness. Legally important and hard to build.
 - **Frontend routing structure** — German URL paths (`/fristen`, `/termine`, `/einstellungen`), nested case detail routes with layout.tsx for shared chrome. Proper use of App Router patterns.
 ### 3.2 Structural Concerns
 - **No database migrations** — the schema was apparently created via SQL scripts run manually. There's a `seed/demo_data.sql` but no migration system. For a production system, this is unsustainable.
 - **No CI/CD pipeline** — no `.github/workflows/`, `.gitea/`, or any CI configuration. Tests run locally but not automatically.
 - **No API versioning** — all routes are at `/api/`. Adding breaking changes will break clients.
 - **Services take raw `*sqlx.DB`** — no transaction support across service boundaries. Creating a case + event is not atomic (if the event insert fails, the case still exists).
 - **Models are just struct definitions** — no validation methods, no constructor functions. Validation is scattered across handlers.
 ### 3.3 Data Model
 Based on the seed data and model files, the schema is reasonable:
 - `tenants`, `user_tenants` (multi-tenancy)
 - `cases`, `parties` (case management)
 - `deadlines`, `appointments` (time management)
 - `documents`, `case_events`, `notes` (supporting data)
 - `proceeding_types`, `deadline_rules`, `holidays` (reference data)
 **Missing indexes likely needed:**
 - `deadlines(tenant_id, status, due_date)` — for dashboard queries
 - `appointments(tenant_id, start_at)` — for calendar queries
 - `case_events(case_id, created_at)` — for event feeds
 - `cases(tenant_id, status)` — for filtered lists
 **Missing constraints:**
 - No CHECK constraint on status values (cases, deadlines, appointments)
 - No UNIQUE constraint on `case_number` per tenant
 - No foreign key from `notes` to the parent entity (if polymorphic)
 ---
 ## 4. Security Assessment
 ### 4.1 Authentication
 - **JWT validation is correct** — algorithm check (HMAC only), expiry check, sub claim extraction. Using `golang-jwt/v5`.
 - **Supabase Auth on frontend** — proper cookie-based session with server-side verification in middleware.
 - **No refresh token rotation** — the API client uses `getSession()` which may serve stale tokens.
 ### 4.2 Authorization
 - **Critical: Tenant isolation bypass** (see 1.1)
 - **No role-based access control** — `user_tenants` has a `role` column but it's never checked. Any member can do anything.
 - **No resource-level permissions** — any user in a tenant can delete any case, document, etc.
 ### 4.3 Input Validation
 - **SQL injection: Protected** — all queries use parameterized placeholders.
 - **XSS: Partially protected** — React auto-escapes, but the API returns raw strings that could contain HTML. The `Content-Disposition` header is vulnerable (see 2.6).
 - **File upload: Partially protected** — `MaxBytesReader` limits to 50MB, but no file type validation (could upload .exe, .html with scripts, etc.).
 - **Rate limiting: AI endpoints only** — the rest of the API has no rate limiting. Login/register go through Supabase (which has its own limits), but all CRUD endpoints are unlimited.
 ### 4.4 Secrets
 - **No hardcoded secrets** — all via environment variables. Good.
 - **CalDAV credentials in plaintext** — see 1.3.
 - **Supabase service key in backend** — necessary for storage, but this key has full DB access. Should be scoped.
 ---
 ## 5. Testing Assessment
 ### 5.1 Backend Tests (15 files)
 - **Integration test** — sets up real DB connection, creates JWT, tests full HTTP flow. Excellent pattern but requires DATABASE_URL (skips otherwise).
 - **Handler tests** — mock-based unit tests for most handlers. Test JSON parsing, error responses, basic happy paths.
 - **Service tests** — deadline calculator has solid date arithmetic tests. Holiday service tested. CalDAV service tested with mocks. AI service tested with mocked HTTP.
 - **Middleware tests** — rate limiter tested.
 - **Auth tests** — tenant resolver tested.
 ### 5.2 Frontend Tests (4 files)
 - `api.test.ts` — tests the API client
 - `DeadlineTrafficLights.test.tsx` — component test
 - `CaseOverviewGrid.test.tsx` — component test
 - `LoginPage.test.tsx` — auth page test
 ### 5.3 What's Missing
 - **No E2E tests** — no Playwright/Cypress. Critical for a law firm app where correctness matters.
 - **No contract tests** — frontend and backend are tested independently. A schema change could break the frontend without any test catching it.
 - **Deadline calculation edge cases** — needs tests for year boundaries, leap years, holidays falling on weekends, multiple consecutive holidays.
 - **Multi-tenant security tests** — no test verifying that User A can't access Tenant B's data. This is the most important test to add.
 - **Frontend test coverage is thin** — 4 tests for ~30 components. The dashboard, all forms, navigation, error states are untested.
 - **No load testing** — unknown how the system behaves under concurrent users.
 ---
 ## 6. UX Assessment
 ### 6.1 What Works
 - **Dashboard is strong** — traffic light deadline indicators, upcoming timeline, case overview, quick actions. A lawyer can see what matters at a glance.
 - **German localization** — UI is in German with proper legal terminology (Akten, Fristen, Termine, Parteien).
 - **Mobile responsive** — sidebar collapses to hamburger menu, layout uses responsive grids.
 - **Loading states** — skeleton screens on dashboard, not just spinners.
 - **Breadcrumbs** — navigation trail on all pages.
 - **Deadline calculator** — unique feature that provides real value for UPC litigation.
 ### 6.2 What a Lawyer Would Stumble On
 1. **No onboarding flow** — after registration, user has no tenant, no cases. The app shows empty states but doesn't guide the user to create a tenant or import data.
 2. **No search** — there's no global search. A lawyer with 100+ cases needs to find things fast.
 3. **No keyboard shortcuts** — power users (lawyers are keyboard-heavy) have no shortcuts.
 4. **Sidebar mixes languages** — "Akten" (German) vs "AI Analyse" (English). Should be consistent.
 5. **No notifications** — overdue deadlines don't trigger any alert beyond the dashboard color. No email alerts, no push notifications.
 6. **No print view** — lawyers need to print deadline lists, case summaries. No print stylesheet.
 7. **No bulk operations** — can't mark multiple deadlines as complete, can't bulk-assign parties.
 8. **Document upload has no preview** — uploaded PDFs can't be viewed inline.
 9. **AI features require manual trigger** — AI summary and deadline extraction are manual. Should auto-trigger on document upload.
 10. **No activity log per user** — no audit trail of who changed what. Critical for law firm compliance.
 ---
 ## 7. Deployment Assessment
 ### 7.1 Docker Setup
 - **Multi-stage builds** — both Dockerfiles use builder pattern. Good.
 - **Backend is minimal** — Alpine + static binary + ca-certificates. ~15MB image.
 - **Frontend** — Bun for deps/build, Node for runtime (standalone output). Reasonable.
 - **Missing:** go.sum not copied in backend Dockerfile (see 2.9).
 - **Missing:** No docker-compose.yml for local development.
 - **Missing:** No health check in Dockerfile (`HEALTHCHECK` instruction).
 ### 7.2 Environment Handling
 - **Config validates required vars** — `DATABASE_URL` and `SUPABASE_JWT_SECRET` are checked at startup.
 - **Supabase URL/keys not validated** — if missing, features silently fail or crash at runtime.
 - **No .env.example** — new developers don't know what env vars are needed.
 ### 7.3 Reliability
 - **No graceful shutdown** (see 2.7)
 - **No readiness/liveness probes** — `/health` exists but only checks DB connectivity. No readiness distinction.
 - **CalDAV sync runs in-process** — if the sync goroutine panics, it takes down the API server.
 - **No structured error recovery** — panics in handlers will crash the process (no recovery middleware).
 ---
 ## 8. Competitive Analysis
 ### 8.1 The Market
 German Kanzleisoftware is a mature, crowded market:
 | Tool | Type | Price | Key Strength |
 |------|------|-------|-------------|
 | **RA-MICRO** | Desktop + Cloud | ~100-200 EUR/user/mo | Market leader, 30+ years, full beA integration |
 | **ADVOWARE** | Desktop + Cloud | from 20 EUR/mo | Budget-friendly, strong for small firms |
 | **AnNoText** (Wolters Kluwer) | Desktop + Cloud | Custom pricing | Enterprise, AI document analysis, DictNow |
 | **Actaport** | Cloud-native | from 79.80 EUR/mo | Modern UI, Mandantenportal, integrated Office |
 | **Haufe Advolux** | Cloud | Custom | User-friendly, full-featured |
 | **Renostar Legal Cloud** | Cloud | Custom | Browser-based, no installation |
 ### 8.2 Table-Stakes Features KanzlAI is Missing
 These are **mandatory** for any German Kanzleisoftware to be taken seriously:
 1. **beA Integration** — since 2022, German lawyers must use the electronic court mailbox (besonderes elektronisches Anwaltspostfach). No Kanzleisoftware sells without it. This is a **massive** implementation effort (KSW-Schnittstelle from BRAK).
 2. **RVG Billing (Gebührenrechner)** — automated fee calculation per RVG (Rechtsanwaltsvergütungsgesetz). Every competitor has this built-in. Without it, lawyers can't bill clients.
 3. **Document Generation** — templates for Schriftsätze, Klageschriften, Mahnbescheide with auto-populated case data. Usually integrated with Word.
 4. **Accounting (FiBu)** — client trust accounts (Fremdgeld), DATEV export, tax-relevant bookkeeping. Legal requirement.
 5. **Conflict Check (Kollisionsprüfung)** — check if the firm has a conflict of interest before taking a case. Legally required (§ 43a BRAO).
 6. **Dictation System** — voice-to-text for lawyers. RA-MICRO has DictaNet, AnNoText has DictNow.
 ### 8.3 Where KanzlAI Could Differentiate
 Despite the feature gap, KanzlAI has some advantages:
 1. **AI-native** — competitors are bolting AI onto 20-year-old software. KanzlAI has Claude API integration from day one. The deadline extraction from PDFs is genuinely useful.
 2. **UPC specialization** — the deadline calculator with UPC Rules of Procedure knowledge is unique. No competitor has deep UPC litigation support.
 3. **CalDAV sync** — bidirectional sync with external calendars is not common in German Kanzleisoftware.
 4. **Modern tech stack** — React + Go + Supabase vs. the .NET/Java/Desktop world of RA-MICRO et al.
 5. **Multi-tenant from day 1** — designed for SaaS, not converted from desktop software.
 ### 8.4 Strategic Recommendation
 **Don't compete head-on with RA-MICRO.** The feature gap is 10+ person-years of work. Instead:
 **Option A: UPC Niche Tool** — Pivot back to UPC patent litigation. Build the best deadline calculator, case tracker, and AI-powered brief analysis tool for UPC practitioners. There are ~1000 UPC practitioners in Europe who need specialized tooling that RA-MICRO doesn't provide. Charge 200-500 EUR/mo.
 **Option B: AI-First Legal Assistant** — Don't call it "Kanzleimanagement." Position as an AI assistant that reads court documents, extracts deadlines, and syncs to the lawyer's existing Kanzleisoftware via CalDAV/iCal. This sidesteps the feature gap entirely.
 **Option C: Full Kanzleisoftware** — If you pursue this, beA integration is the first priority, then RVG billing. Without these two, no German lawyer will switch.
 ---
 ## 9. Strengths (What's Good, Keep Doing It)
 1. **Architecture is solid** — the Go + Next.js + Supabase stack is well-chosen. Clean separation of concerns.
 2. **SQL is safe** — parameterized queries throughout. No injection vectors.
 3. **Multi-tenant design** — tenant_id scoping with RLS is the right approach.
 4. **CalDAV implementation** — genuinely impressive for an MVP. Bidirectional sync with conflict resolution.
 5. **Deadline calculator** — ported from youpc.org with holiday awareness. Real domain value.
 6. **AI integration** — Claude API with tool use for structured extraction. Clean implementation.
 7. **Dashboard UX** — traffic lights, timeline, quick actions. Lawyers will get this immediately.
 8. **German-first** — proper legal terminology, German date formats, localized UI.
 9. **Test foundation** — 15 backend test files with integration tests. Good starting point.
 10. **Docker builds are lean** — multi-stage, Alpine-based, standalone Next.js output.
 ---
 ## 10. Priority Roadmap
 ### P0 — This Week
 - [ ] Fix tenant isolation bypass in TenantResolver (1.1)
 - [ ] Consolidate tenant resolution logic (1.2)
 - [ ] Encrypt CalDAV credentials at rest (1.3)
 - [ ] Add CORS middleware + security headers (1.4)
 - [ ] Stop leaking internal errors to clients (1.5)
 - [ ] Fix Dockerfile go.sum copy (2.9)
 ### P1 — Before Demo/Beta
 - [ ] Add input validation (length limits, allowed values) (2.1)
 - [ ] Add pagination to all list endpoints (2.2)
 - [ ] Fix `search_path` connection pool issue (2.8)
 - [ ] Add graceful shutdown with signal handling (2.7)
 - [ ] Sanitize Content-Disposition filename (2.6)
 - [ ] Add multi-tenant security tests
 - [ ] Add database migrations system
 - [ ] Add `.env.example` file
 - [ ] Add onboarding flow for new users
 ### P2 — Next Iteration
 - [ ] Role-based access control (admin/member/readonly)
 - [ ] Global search
 - [ ] Email notifications for overdue deadlines
 - [ ] Audit trail / activity log per user
 - [ ] Auto-trigger AI extraction on document upload
 - [ ] Print-friendly views
 - [ ] E2E tests with Playwright
 - [ ] CI/CD pipeline
 ### P3 — Strategic
 - [ ] Decide market positioning (UPC niche vs. AI assistant vs. full Kanzleisoftware)
 - [ ] If Kanzleisoftware: begin beA integration research
 - [ ] If Kanzleisoftware: RVG Gebührenrechner
 - [ ] If UPC niche: integrate lex-research case law database
 ---
 *This audit was conducted by reading every source file in the repository, running all tests, analyzing the database schema via seed data, and comparing against established German Kanzleisoftware competitors.*