crm-system/security-review-phase2.md

LeoCRM Phase 2 — Security & Data Risk Review

Reviewer: Security Data Engineer (A0 Orchestrator)
Date: 2026-06-28
Project: leocrm
Phase: Pre-Implementation (Phase 2 to Phase 3)
Files reviewed: architecture.md (1939 lines), task_graph.json (v2.0.0, 13 tasks), requirements.md (2142 lines, 143 features)
Scope: Security architecture, multi-tenant isolation, auth, data persistence, migration, backup/restore, plugin security, dependency risks

---

VERDICT: APPROVED_WITH_CONCERNS

The architecture is well-structured with strong fundamentals (session-based auth, CSRF protection, CSP headers, RBAC with field-level permissions, audit trail). However, 7 major risks and 8 minor risks must be addressed before or during implementation. No critical blocking issues found, but 3 major risks (RLS gap, rate limiting, CORS) should be resolved before Phase 3 start.

Severity	Count
Critical	0
Major	7
Minor	8

---

1. AUTH SECURITY — Session-Based Auth + API Tokens

Design Summary

• Session store: Redis (session:{id}, TTL=8h) — primary runtime store
• Audit trail: PostgreSQL sessions table (immutable, retains all sessions ever created)
• Cookie: leocrm_session=<id>; HttpOnly; Secure; SameSite=Strict; Path=/
• Password hashing: bcrypt cost=12
• API tokens: api_tokens table (SHA-256 hashed, scoped, expiring) — post-MVP but architecture-ready
• Password reset: Token-based, 24h expiry, hashed storage, no user enumeration, session invalidation on reset

Assessment: GOOD with concerns

Positive:

• ADR-05 decision is sound: server-side sessions avoid JWT pitfalls (token leakage, no revocation)
• Immediate session invalidation via Redis key deletion
• Forensic session audit trail in PostgreSQL (session validation flow checks Redis then PG audit then deny)
• No user enumeration on login or password reset
• Cookie flags correctly set (HttpOnly, Secure, SameSite=Strict)

MAJOR RISK M-01: No brute-force protection on auth endpoints

• Finding: No account lockout, failed-attempt tracking, or rate limiting on POST /api/v1/auth/login or POST /api/v1/auth/password-reset/request
• Impact: An attacker can perform unlimited password guessing attempts. bcrypt cost=12 slows each attempt (~250ms) but does not prevent distributed attacks.
• Requirements reference: F-AUTH-01 has no lockout test scenario. Non-Goals section 18 explicitly excludes rate limiting from v1.
• Recommendation: Add a minimal failed-attempt counter in Redis (login_failures:{email}, TTL=15min, threshold=10, lockout 15min) before Phase 3. This is distinct from general rate limiting and is scoped to auth only.

MINOR RISK m-01: LEOCRM_SECRET_KEY purpose and rotation undefined

• Finding: LEOCRM_SECRET_KEY=<min-32-chars> is listed in env vars but its usage is not specified (session ID generation? cookie signing? CSRF token generation?). No rotation policy documented.
• Recommendation: Document the secret's purpose in .env.example and define a rotation procedure in the admin guide. If used for signing session IDs, rotation invalidates all sessions (acceptable, document it).

MINOR RISK m-02: No 2FA in v1

• Finding: Non-Goals section 13 explicitly excludes 2FA. Acceptable for v1 internal CRM, but should be prioritized post-MVP if exposed to internet.
• Recommendation: Document as post-MVP roadmap item with priority based on exposure.

---

2. MULTI-TENANT ISOLATION — Row-Level Security

Design Summary

• Every table has tenant_id (UUID, NOT NULL on core tables)
• ORM auto-filtering via SQLAlchemy before_query event listener (do_orm_execute)
• TenantMixin base class enforces tenant_id column on all models
• Cross-tenant access returns 404 (not 403) to prevent information leakage
• Plugin tables must include tenant_id (validator checks)
• Tenant switch via POST /api/v1/auth/switch-tenant

Assessment: MODERATE RISK — needs DB-level enforcement

MAJOR RISK M-02: RLS claimed but only ORM-level filtering implemented

• Finding: Architecture line 154 states "PostgreSQL 16 with Row-Level Security for tenant isolation" but the implementation (lines 1232-1240) is exclusively ORM-level filtering via SQLAlchemy event listener. No CREATE POLICY, ENABLE ROW LEVEL SECURITY, or SET app.current_tenant session variables are defined.
• Impact: Any query that bypasses the ORM (raw SQL, session.execute(text(...)), stored procedures, Alembic migrations, ARQ worker jobs that don't set tenant context) will NOT be tenant-filtered. A single missed _tenant_filter_disabled flag or raw query can leak cross-tenant data.
• Evidence: The before_query listener checks if not _tenant_filter_disabled — this flag must be managed carefully. Any code path that sets it without restoring is a data leak vector.
• Recommendation:
  1. Implement PostgreSQL RLS policies as a defense-in-depth layer: CREATE POLICY tenant_isolation ON <table> USING (tenant_id = current_setting('app.current_tenant')::uuid)
  2. Set app.current_tenant at the beginning of each DB session/transaction from the authenticated session context
  3. Keep ORM filtering as the primary layer; RLS as the safety net
  4. This is a design change — should be approved before Phase 3 implementation

MAJOR RISK M-03: Tenant context propagation to ARQ workers not defined

• Finding: Background jobs (exports, mail-sync, reminders, backups) run in a separate worker process. The architecture does not specify how current_tenant_id() is set in worker context. If a worker job operates on tenant-scoped data without setting the tenant context, the ORM filter may not apply or may apply incorrectly.
• Impact: Cross-tenant data exposure in background job results (e.g., export contains data from all tenants).
• Recommendation: Define and document tenant context propagation for ARQ workers: each job must carry tenant_id in its job context, and the worker must set current_tenant_id() before executing any DB queries.

MINOR RISK m-03: Tenant switch does not validate user-tenant membership

• Finding: POST /api/v1/auth/switch-tenant updates the session's tenant_id. The architecture does not explicitly state that the endpoint validates the user's membership in the target tenant via user_tenants table.
• Recommendation: Ensure the switch endpoint checks user_tenants membership before updating the session. Add a test case: user attempts to switch to a tenant they don't belong to then 403.

---

3. CSRF PROTECTION

Design Summary

• SameSite=Strict cookie (browser-level protection)
• Origin-Header-Validierung middleware (server-side check)
• Only GET/HEAD/OPTIONS exempt from CSRF check
• CSRF token stored per session in Redis and PostgreSQL audit table

Assessment: GOOD

Positive:

• Two-layer CSRF protection (SameSite + Origin validation) is a solid approach
• SameSite=Strict is the strongest browser-level CSRF defense
• Origin validation is server-side and not bypassable by client tweaks
• Test scenarios defined in task_graph.json: "CSRF: POST without Origin header then 403"

MINOR RISK m-04: CSRF token stored but not validated in requests

• Finding: A csrf_token is generated and stored per session, but the architecture does not describe a mechanism where the frontend sends the token back (e.g., in a X-CSRF-Token header) and the backend validates it. The protection relies entirely on SameSite + Origin.
• Impact: SameSite=Strict + Origin validation is sufficient for v1. The stored CSRF token appears unused.
• Recommendation: Either (a) remove the csrf_token from the session model if SameSite+Origin is the chosen strategy, or (b) implement double-submit cookie pattern for defense-in-depth. Clarify in architecture.

---

4. INPUT VALIDATION

Design Summary

• Pydantic schemas validate all API inputs (F-DATA-03)
• XSS protection: server-side Pydantic validation + frontend DOMPurify/escaped rendering (F-SEC-02)
• CSP headers prevent inline script execution
• HTML in user inputs is escaped, not rendered

Assessment: GOOD

Positive:

• Pydantic on all API inputs is FastAPI best practice
• Server-side + client-side sanitization (defense in depth)
• CSP header is well-configured: script-src 'self', object-src 'none', base-uri 'self'
• XSS test scenarios defined in requirements

MINOR RISK m-05: No SQL injection prevention explicitly documented

• Finding: While SQLAlchemy ORM with parameterized queries is the default, the architecture does not explicitly state a prohibition on raw SQL or string interpolation in queries.
• Recommendation: Add an explicit coding guideline: no raw SQL with string interpolation; all raw queries must use parameterized text() with bind parameters.

---

5. SECRETS MANAGEMENT (F-ENV-01)

Design Summary

• .env.example documents all environment variables with <secret> placeholders
• Secrets never in Git repo (.gitignore includes .env)
• Missing secret env var then app fails to start with clear error (F-ENV-01 test scenario 3)
• Secrets: POSTGRES_PASSWORD, LEOCRM_SECRET_KEY, SMTP_PASS, MAIL_ENCRYPTION_KEY, S3_SECRET_KEY, AI_API_KEY
• Pydantic Settings for env var loading (config.py)

Assessment: ADEQUATE for v1 with gaps

MAJOR RISK M-04: No secret rotation policy

• Finding: No rotation procedure is defined for any secret (LEOCRM_SECRET_KEY, MAIL_ENCRYPTION_KEY, POSTGRES_PASSWORD, SMTP_PASS). F-ENV-01 only covers initial setup, not lifecycle.
• Impact: If a secret is compromised, there is no documented procedure to rotate it. MAIL_ENCRYPTION_KEY rotation is especially critical — changing it without a re-encryption plan would make existing encrypted mail credentials unreadable.
• Recommendation:
  1. Document rotation procedures for each secret in admin guide
  2. For MAIL_ENCRYPTION_KEY: implement key versioning (store key_id with encrypted data, support old + new key during rotation)
  3. For LEOCRM_SECRET_KEY: document that rotation invalidates all sessions (acceptable)
  4. For POSTGRES_PASSWORD: document procedure (change password, update env, restart)

MINOR RISK m-06: .env file approach for production

• Finding: Docker Compose uses env_file: .env for all services including production on Coolify. This means secrets are stored in a plaintext file on the server.
• Impact: If the host filesystem is compromised, all secrets are readable. Docker env vars are also visible via docker inspect.
• Recommendation: For production on Coolify, use Coolify's secret/environment variable management (injects as container env vars without a file on disk). The .env file approach is fine for dev only. Document this split in the deployment guide.

---

6. DATA MIGRATION RISK (F-MIG-01)

Design Summary

• F-MIG-01: CSV import with field mapping, per-row error reporting, auto-company-detection for contact imports
• No legacy system data migration (no ETL from external CRM systems)
• No schema migration risk (greenfield project with Alembic)

Assessment: LOW RISK — properly scoped

Positive:

• CSV import is well-defined with field mapping and error handling
• No complex legacy migration in v1 (correct scope decision)
• Alembic for schema migrations is standard and reliable

MAJOR RISK M-05: CSV import has no file size limit or row count validation

• Finding: F-MIG-01 test scenario imports 50 companies. No mention of maximum file size, maximum row count, or memory protection for large CSV files. A 500MB CSV with 1M rows could cause OOM or timeout.
• Impact: Denial of service via large CSV upload; potential memory exhaustion.
• Recommendation:
  1. Define max upload size (e.g., 10MB for CSV)
  2. Process CSV in streaming mode (not loading entire file into memory)
  3. Add row count limit (e.g., 50,000 rows per import)
  4. Run import as background job (ARQ) for files >1000 rows

---

7. BACKUP/RESTORE (F-INFRA-02)

Design Summary

• pg_dump daily cron job to backup volume or S3
• Storage volume backup (files)
• Restore documented in docs/admin-guide.md
• Backup failure triggers alert to admin

Assessment: MAJOR RISK — inadequate for multi-tenant production

MAJOR RISK M-06: Backup strategy insufficient for multi-tenant PostgreSQL

• Finding: The backup design has multiple gaps:
  1. No backup encryption: pg_dump output is plaintext. Tenant data (companies, contacts, emails) is stored unencrypted in the backup volume/S3.
  2. No retention policy: No definition of how many backups to keep (7 days? 30 days?). Unlimited backups cause storage exhaustion; too few cause data loss.
  3. No tested restore procedure: F-INFRA-02 acceptance criterion says "Restore-Dokumentation vorhanden" but there is no test scenario that verifies an actual restore works.
  4. No point-in-time recovery: Only daily pg_dump snapshots. If a tenant accidentally deletes data at 14:00 and notices at 17:00, all data created between 00:00 and 14:00 that day is lost.
  5. Multi-tenant restore granularity: pg_dump is all-or-nothing. If one tenant needs restore, all tenants are affected. No mention of tenant-level export/restore.
  6. Redis not backed up: Session data is in Redis with TTL=8h. Redis is not included in backup strategy. If Redis is lost, all active sessions are invalidated (users must re-login). This is acceptable but should be documented.
• Recommendation:
  1. Encrypt pg_dump output (gpg or S3 SSE-KMS)
  2. Define retention: 7 daily + 4 weekly + 12 monthly
  3. Add a restore test to the test suite (backup, restore, verify row count)
  4. Enable PostgreSQL WAL archiving for point-in-time recovery (PITR)
  5. Document that restore is all-tenant; consider tenant-level CSV export as a quick-recovery alternative
  6. Document Redis session loss behavior (acceptable: users re-login)

---

8. PLUGIN SECURITY (T03 Plugin Framework)

Design Summary

• Built-in plugins only (no dynamic external loading in v1) — ADR-03
• Plugin manifest schema (Pydantic)
• Lifecycle hooks: install/activate/deactivate/uninstall
• Plugin DB migration runner with plugin_migrations tracking
• Migration validator checks tenant_id on all plugin tables
• Service Container DI: plugins receive db, cache, event_bus, storage, notifications
• Event Bus integration: plugins register/unregister event listeners

Assessment: MODERATE RISK — tenant isolation enforced, but no permission scoping

MAJOR RISK M-07: No plugin API permission scoping

• Finding: Plugins receive injected services (db, cache, event_bus, storage, notifications) but there is no permission model restricting what a plugin can do. A plugin with access to the db session can query any table within the current tenant context. There is no "plugin A can only read companies, plugin B can only write to its own tables" model.
• Impact: A malicious or buggy built-in plugin could access/modify data from other modules within the same tenant. Since all v1 plugins are built-in (shipped with code), this is lower risk, but the architecture should define the permission model for when external plugins are added post-MVP.
• Recommendation:
  1. For v1: document that plugins are trusted (built-in only) and have full tenant-scoped access
  2. For post-MVP: define plugin permission scopes in the manifest (e.g., permissions: ["companies:read", "contacts:write"])
  3. Add a test: plugin cannot access data from a different tenant (already covered by tenant_id validator)

MINOR RISK m-07: Plugin event bus has no namespacing

• Finding: Plugins register event listeners on a shared event bus. There is no mention of event namespacing to prevent event name collisions between plugins.
• Recommendation: Use prefixed event names (e.g., dms.file.uploaded, calendar.event.created) to avoid collisions.

MINOR RISK m-08: Plugin uninstall with data removal has no confirmation audit

• Finding: DELETE /api/v1/plugins/{name}?remove_data=true drops plugin tables. The architecture does not mention that this destructive action is logged in the audit log.
• Recommendation: Log plugin uninstall with data removal to audit_log with actor, timestamp, plugin name, and table list.

---

9. DEPENDENCY RISKS

Design Summary

• Backend: FastAPI, SQLAlchemy, Pydantic, ARQ, Redis-py, asyncpg/psycopg
• Frontend: React 18, TanStack Query v5, Vite, Tailwind CSS
• Database: PostgreSQL 16-alpine
• Cache/Queue: Redis 7-alpine
• Document editing: OnlyOffice Document Server

Assessment: LOW-MODERATE RISK

OnlyOffice :latest tag

• Finding: Docker Compose uses onlyoffice/documentserver:latest. This tag is mutable and can introduce breaking changes or security vulnerabilities without notice.
• Impact: Unpredictable updates; potential breaking changes; supply chain risk.
• Recommendation: Pin to a specific version tag (e.g., onlyoffice/documentserver:8.2.2). Update deliberately after testing.

Other dependency notes:

• FastAPI, React 18, PostgreSQL 16, Redis 7 are all current stable major versions with active security maintenance
• No known critical CVEs in these major versions as of 2026-06
• Recommendation: Pin all dependencies in requirements.txt / package.json with exact versions or minimum patches. Add pip-audit and npm audit to CI pipeline.
• Recommendation: Use postgres:16-alpine and redis:7-alpine (already specified — good). Pin minor versions for reproducibility.

---

10. RATE LIMITING

Assessment: MAJOR RISK — explicitly excluded from v1

Finding: Non-Goals section 18: "Kein zentrales Rate-Limiting in v1." This means:

• POST /api/v1/auth/login — no rate limit (brute-force possible, see M-01)
• POST /api/v1/auth/password-reset/request — no rate limit (email bombing possible)
• All API endpoints — no rate limit (DoS via excessive requests)
• API tokens (post-MVP) — no rate limit per token

Impact:

• Auth endpoints are brute-force vulnerable (mitigated partially by bcrypt cost=12, but not for distributed attacks)
• Password reset endpoint can be abused to send unlimited emails (SMTP abuse, email bombing)
• General API abuse (data scraping, DoS)

Recommendation:

• Implement auth-scoped rate limiting (not general rate limiting) before Phase 3:
  • Login: 10 attempts per email per 15 min (Redis counter)
  • Password reset: 3 requests per email per hour
  • This is minimal effort and high security value
• General API rate limiting can remain post-MVP if the app is internal-only, but document the decision

---

11. CORS

MAJOR RISK M-08: CORS configuration not specified

• Finding: The frontend is served on port 80 (Nginx) and the API on port 8000 (FastAPI). In production, they may share a domain (reverse proxy) or be on separate ports. The architecture does not specify CORS headers.
• Impact: If frontend and API are on different origins (e.g., dev environment: localhost:80 to localhost:8000), the browser will block requests without proper CORS headers. If CORS is set to wildcard, credentials (cookies) will not work.
• Recommendation:
  • In production: serve frontend + API behind the same reverse proxy (same origin, no CORS needed)
  • In development: configure FastAPI CORS middleware with allow_origins=["http://localhost:80"], allow_credentials=True, allow_methods=["*"], allow_headers=["*"]
  • Never use allow_origins=["*"] with allow_credentials=True (browser rejects this)
  • Document CORS configuration in architecture.md

---

12. DOCKER/COMPOSE SECURITY

Findings

Issue	Severity	Detail
No non-root user	Minor	All containers run as root by default. Add user: directive or use images with non-root users.
No cap_drop: ALL	Minor	Containers retain all Linux capabilities. Drop all and add only needed ones.
No read-only filesystem	Minor	Add read_only: true with tmpfs for writable paths.
Redis without auth	Major	redis:7-alpine has no requirepass or ACL configured. Any container on the network can access Redis.
OnlyOffice :latest	Minor	Mutable tag; pin to specific version.
All ports exposed	Minor	ports: ["8000:8000"], ["80:80"], ["8080:80"] expose to host. In production, use internal network + reverse proxy only.
No health checks on all services	Minor	Only backend has a healthcheck. Add for postgres, redis, worker.
No resource limits	Minor	No mem_limit, cpus limits. A runaway process can consume all host resources.

Recommendation for Redis auth:

• Add REDIS_PASSWORD env var
• Configure Redis with requirepass or use ACL users
• Update REDIS_URL to include password: redis://:<password>@redis:6379/0

---

13. FILE UPLOADS

Finding

DMS plugin (T05) handles file uploads via POST /api/v1/dms/files/upload (multipart). The architecture does not specify:

• Maximum file size limit
• Allowed file types / MIME type validation
• File content verification (magic bytes, not just extension)
• Malware / virus scanning
• Filename sanitization (path traversal prevention)

Assessment: MAJOR RISK (deferred to plugin implementation)

• Impact: Path traversal via malicious filenames, disk exhaustion via large files, stored XSS via uploaded HTML/SVG files, potential malware storage.
• Recommendation: Define upload security in T05 task specification:
  1. Max file size: 50MB (configurable)
  2. Allowed MIME types whitelist (exclude text/html, image/svg+xml, application/javascript)
  3. Filename sanitization: strip path components, use UUID-based storage names
  4. Store files outside web root (already handled by storage service)
  5. Post-MVP: ClamAV integration for malware scanning

---

14. LOGGING SENSITIVE DATA

Assessment: GOOD

• Structured JSON logs (F-INFRA-03): timestamp, level, event, method, path, status, duration, tenant_id, user_id
• No password, token, or secret values in log format
• Log level configurable via LOG_LEVEL env var
• Recommendation: Add explicit log sanitization in the logging middleware: filter out password, new_password, token, Authorization header fields from request body logging if request body is ever logged.

---

15. DATA PERSISTENCE AND DATA LOSS RISK

Assessment: MODERATE RISK

• Soft-delete with deleted_at column — good for accidental deletion recovery
• DSGVO hard-delete with deletion_log — good for compliance
• deletion_log mentioned in architecture but table schema not fully defined in the reviewed sections
• Risk: Soft-deleted data is still in the database. If a tenant requests GDPR deletion, the hard-delete must also remove soft-deleted records.
• Recommendation: Verify deletion_log table schema includes: tenant_id, entity_type, entity_id, deleted_by, deleted_at, data_summary (for audit).

---

SUMMARY TABLE

#	Risk	Severity	Domain	Action Before Phase 3?
M-01	No brute-force protection on auth	Major	Auth	YES — add Redis-based attempt counter
M-02	RLS claimed but ORM-only filtering	Major	Multi-Tenant	YES — add DB-level RLS as defense-in-depth
M-03	ARQ worker tenant context undefined	Major	Multi-Tenant	YES — define in architecture
M-04	No secret rotation policy	Major	Secrets	NO — document before deployment
M-05	CSV import no size/row limit	Major	Migration	NO — add in T05/T07 implementation
M-06	Backup insufficient for multi-tenant	Major	Backup	NO — resolve before deployment
M-07	No plugin API permission scoping	Major	Plugins	NO — acceptable for v1 (built-in only)
M-08	CORS not specified	Major	Network	YES — configure and document
m-01	LEOCRM_SECRET_KEY purpose/rotation undefined	Minor	Secrets	NO
m-02	No 2FA in v1	Minor	Auth	NO (post-MVP)
m-03	Tenant switch membership validation	Minor	Multi-Tenant	YES — add test case
m-04	CSRF token stored but unused	Minor	CSRF	NO — clarify architecture
m-05	No SQL injection prevention guideline	Minor	Validation	NO — add coding guideline
m-06	.env file for production	Minor	Secrets	NO — use Coolify env management
m-07	Plugin event bus no namespacing	Minor	Plugins	NO
m-08	Plugin uninstall no audit log	Minor	Plugins	NO

---

TOP 3 RISKS

1. M-02: RLS gap — Architecture claims PostgreSQL RLS but implements only ORM-level tenant filtering. Raw SQL, worker jobs, or filter bypass bugs can leak cross-tenant data. Must add DB-level RLS policies as defense-in-depth before implementation.

2. M-01: No brute-force protection — Auth endpoints (login, password reset) have no rate limiting, lockout, or failed-attempt tracking. Combined with M-08 (no CORS config), the attack surface for credential attacks is significant. Must add minimal Redis-based auth rate limiting before Phase 3.

3. M-06: Backup strategy inadequate — No encryption, no retention policy, no tested restore, no PITR for multi-tenant PostgreSQL. Data loss risk for production tenants. Must resolve before deployment phase.

---

RECOMMENDATION FOR PHASE 3 START

APPROVED_WITH_CONCERNS — Phase 3 may start after addressing the 3 pre-implementation items:

1. M-02: Add PostgreSQL RLS policy definitions to architecture.md (defense-in-depth alongside ORM filtering)
2. M-01 + Rate Limiting: Add auth-scoped rate limiting (login attempt counter + password reset throttle) to T01 task specification
3. M-08: Add CORS configuration to architecture.md (same-origin in prod, explicit origins in dev)

Additionally, update T01 task to include:

• ARQ worker tenant context propagation (M-03)
• Tenant switch membership validation test (m-03)
• Redis auth configuration (Docker Compose)

The remaining major risks (M-04, M-05, M-06, M-07) can be addressed during implementation or before deployment.

---

Review complete. No secrets, credentials, or live values were inspected. All findings based on architecture.md, task_graph.json, and requirements.md content only.