Agent Zero
5ec1fc9b05
B1: Remove duplicate get_redis() — singleton no longer overwritten B2: Plugin routes now enforce activation status via require_active_plugin() B3: Fix UploadFile ForwardRef error — remove functools.wraps from wrap_plugin_route B4: DMS upload uses true streaming via save_stream() instead of RAM accumulation B5: Worker on_startup registers plugin event handlers + webhook dispatcher B6: Implement send_password_reset_email job, remove raw token logging B7: Webhook SSRF protection (IP validation, no redirects), secret removed from response B8: RLS repair migration 0044 + separate crm_runtime DB user (NOSUPERUSER, NOBYPASSRLS) B9: Fix .env.docker.example AUTH_SECRET → SECRET_KEY B10: Remove Redis default password, remove exposed DB/Redis ports Also: add frontend_url to config, add SMTP settings to .env.docker.example, update prestart.sh to use MIGRATION_DATABASE_URL for alembic.
201 lines
17 KiB
Markdown
201 lines
17 KiB
Markdown
# LeoCRM Security & Data Risk Assessment
|
|
|
|
**Date:** 2026-07-26
|
|
**Assessor:** Security Data Engineer (A0 Orchestrator)
|
|
**Project:** LeoCRM at `/a0/usr/workdir/leocrm-fix`
|
|
|
|
---
|
|
|
|
## Summary
|
|
|
|
| Severity | Count |
|
|
|----------|-------|
|
|
| CRITICAL | 5 |
|
|
| HIGH | 8 |
|
|
| MEDIUM | 8 |
|
|
| LOW | 5 |
|
|
| **Total**| **26**|
|
|
|
|
---
|
|
|
|
## CRITICAL Issues
|
|
|
|
### C-1: Redis Default Password `changeme` in docker-compose.yml
|
|
**File:** `docker-compose.yml:53`
|
|
**Risk:** Redis stores session data, CSRF tokens, and rate-limit counters. The default password `changeme` is trivially guessable. If Redis port 6379 is exposed, an attacker can read/modify all sessions, steal CSRF tokens, and bypass rate limits.
|
|
**Remediation:** Remove the default fallback. Require `REDIS_PASSWORD` as a mandatory variable (`${REDIS_PASSWORD:?REDIS_PASSWORD is required}`). Use a strong randomly generated password in production.
|
|
|
|
### C-2: No SECRET_KEY in `.env` — Insecure Default Active in Development
|
|
**File:** `.env` (missing `SECRET_KEY`), `app/config.py:55`
|
|
**Risk:** `.env` has no `SECRET_KEY`. The config defaults to `"change-me-in-production-use-a-secure-random-string"`. While `get_settings()` raises in production mode, `.env` sets `ENVIRONMENT=development`, so the default key is silently used. Any signing/token operation using `secret_key` is compromised.
|
|
**Remediation:** Add a strong random `SECRET_KEY` (min 32 chars) to `.env`. Fail-fast in all environments if the default key is detected, not just production.
|
|
|
|
### C-3: PostgreSQL and Redis Ports Exposed to Host
|
|
**File:** `docker-compose.yml:37-38, 56-57`
|
|
**Risk:** `ports: "5432:5432"` and `ports: "6379:6379"` expose the database and Redis to the host network. Combined with weak/default credentials, this allows direct external access to all session data and the entire database.
|
|
**Remediation:** Remove port mappings for production. Use Docker internal networking only (`crm-net`). If debug access is needed, bind to `127.0.0.1:5432:5432` and document it as dev-only.
|
|
|
|
### C-4: Unauthenticated Error Endpoint Forwards Data to External Forgejo
|
|
**File:** `app/routes/errors.py:54-90`, `app/plugins/builtins/forgejo_error_reporter/service.py:151-250`
|
|
**Risk:** The `/api/v1/errors` endpoint requires no authentication. CSRF middleware explicitly bypasses token checks for this path (line 48 of `middleware.py`). Any unauthenticated attacker can POST arbitrary error data (message, stack, URL, userAgent, and **arbitrary context dict**) which gets forwarded to an external Forgejo instance as a public issue. The `context` field accepts `dict[str, Any]` with no size limit on individual keys — an attacker can exfiltrate data or inject malicious content into Forgejo issues.
|
|
**Remediation:** Require authentication for error reporting. If unauthenticated errors are needed, strip the `context` field entirely, add strict schema validation with size limits on all fields, and add a CAPTCHA or stricter rate limiting.
|
|
|
|
### C-5: Plaintext Database Password in `.env`
|
|
**File:** `.env:1`
|
|
**Risk:** `DATABASE_URL=postgresql+asyncpg://leocrm:leocrm@localhost:5432/leocrm` embeds the DB password `leocrm` in plaintext. While `.gitignore` covers `.env`, the password is weak and identical to the username. If the file is accessed via any path traversal, backup leak, or container escape, the database is fully compromised.
|
|
**Remediation:** Use a strong unique password. Separate `DATABASE_URL` construction from credential storage where possible (e.g., use individual `POSTGRES_USER`, `POSTGRES_PASSWORD`, `POSTGRES_HOST`, `POSTGRES_DB` env vars and construct the URL in code).
|
|
|
|
---
|
|
|
|
## HIGH Issues
|
|
|
|
### H-1: Rate Limiter Trusts X-Forwarded-For Without Validation
|
|
**File:** `app/core/rate_limit.py:43-45`
|
|
**Risk:** `get_client_ip()` blindly trusts the `X-Forwarded-For` header. An attacker can set arbitrary values to bypass rate limits on login, password reset, and other endpoints. Each request with a different spoofed IP creates a new rate-limit counter.
|
|
**Remediation:** Only trust `X-Forwarded-For` from known proxy IPs. Configure a trusted proxy list and validate the header chain. Use Starlette's `ProxyHeadersMiddleware` or validate against a `TRUSTED_PROXIES` env var.
|
|
|
|
### H-2: Duplicate `get_redis()` Functions — Connection Leak
|
|
**File:** `app/core/auth.py:53-66` and `app/core/auth.py:94-96`
|
|
**Risk:** Two `get_redis()` functions exist. The first (line 53) returns a singleton. The second (line 94) creates a **new Redis connection on every call**. Code importing `get_redis` may use either version. The middleware (line 69) creates its own Redis connection per request. This leads to connection pool exhaustion under load.
|
|
**Remediation:** Remove the second `get_redis()` (line 94-96). Ensure all code uses the singleton version. The middleware should use `get_redis()` from `app.core.auth` instead of creating its own connection.
|
|
|
|
### H-3: CSRF Middleware Creates New Redis Connection Per Request
|
|
**File:** `app/core/middleware.py:69-90`
|
|
**Risk:** For every unsafe HTTP request, the middleware creates a new `aioredis.from_url()` connection, uses it, then closes it. Under load, this creates thousands of connections and can exhaust Redis connection limits.
|
|
**Remediation:** Use the global Redis singleton via `from app.core.auth import get_redis`. Remove the per-request connection creation and the `finally: await redis.close()` block.
|
|
|
|
### H-4: CSRF Token Stored Plaintext in PostgreSQL
|
|
**File:** `app/core/auth.py:141` (`SessionModel` stores `csrf_token`)
|
|
**Risk:** The CSRF token is stored as plaintext in the PostgreSQL `sessions` table (audit trail). If the database is compromised, all active CSRF tokens are available for CSRF attacks.
|
|
**Remediation:** Store only a hash of the CSRF token in PostgreSQL (like `hash_token()` already exists for session tokens). Compare hashes during validation.
|
|
|
|
### H-5: No File Upload Validation in Storage Backend
|
|
**File:** `app/core/storage.py:69-128`
|
|
**Risk:** `LocalStorage` performs no validation on uploaded files:
|
|
- No path traversal protection: `os.path.join(self.base_path, path)` with a malicious `path` containing `../../` can write anywhere on the filesystem
|
|
- No file type/extension whitelist
|
|
- No file size limit
|
|
- No content-type validation
|
|
- `get_url()` returns the full filesystem path, leaking internal directory structure
|
|
**Remediation:** Sanitize `path` with `os.path.realpath()` and verify it's within `base_path`. Enforce file size limits, extension whitelist, and MIME type validation. Return relative paths from `get_url()`, not absolute filesystem paths.
|
|
|
|
### H-6: WebSocket Connections Lack Authentication Verification
|
|
**File:** `app/plugins/builtins/kommunikation/websocket_manager.py:23-28`, `app/plugins/builtins/ai_ui_control/websocket_manager.py:40-46`
|
|
**Risk:** Both WebSocket managers accept connections via `connect(websocket, user_id)` without verifying that `user_id` is authenticated. The security depends entirely on the calling route. If any WebSocket route passes an untrusted `user_id` (e.g., from query params), an attacker can impersonate any user. There is also no origin verification on WebSocket connections.
|
|
**Remediation:** Verify session cookie inside `connect()` before `websocket.accept()`. Validate the `Origin` header against allowed CORS origins. Add authentication middleware for WebSocket routes.
|
|
|
|
### H-7: In-Memory Rate Limiter in Error Endpoint — Fails with Multiple Workers
|
|
**File:** `app/routes/errors.py:21-40`
|
|
**Risk:** The error endpoint uses a process-local `defaultdict(deque)` for rate limiting. With multiple Uvicorn workers (common in production), each worker has its own counter. An attacker can make `RATE_LIMIT * num_workers` requests per minute.
|
|
**Remediation:** Use the Redis-based `check_rate_limit()` from `app/core/rate_limit.py` instead of the in-memory implementation.
|
|
|
|
### H-8: No CSRF Protection on WebSocket Connections
|
|
**File:** Both WebSocket managers
|
|
**Risk:** WebSocket connections are not protected against CSRF. A malicious site can open a WebSocket to the CRM backend via JavaScript `new WebSocket()` and send commands as the authenticated user (cookies are sent automatically with SameSite=Strict for same-site, but cross-site WebSocket hijacking is still possible if SameSite is configured differently or cookies are sent via `credentials`).
|
|
**Remediation:** Verify the `Origin` header on WebSocket upgrade requests. Reject connections from untrusted origins.
|
|
|
|
---
|
|
|
|
## MEDIUM Issues
|
|
|
|
### M-1: Login Response Leaks `is_system_admin` Flag
|
|
**File:** `app/routes/auth.py:78`
|
|
**Risk:** The login response includes `"is_system_admin": user.is_system_admin`. An attacker who compromises a session or intercepts the response knows whether the account has system-wide privileges, enabling targeted attacks.
|
|
**Remediation:** Do not include `is_system_admin` in the login response. The frontend can determine admin status via the `/me/permissions` endpoint.
|
|
|
|
### M-2: Weak Password Validation — No Complexity Requirements
|
|
**File:** `app/schemas/auth.py:10` (login: `min_length=1`), `app/schemas/user.py:11` (create: `min_length=8`)
|
|
**Risk:** Login accepts any password length (min_length=1). User creation requires min 8 chars but no complexity (uppercase, lowercase, digits, special chars). Users can set passwords like `aaaaaaaa`.
|
|
**Remediation:** Add password complexity validation (min 12 chars, mixed case, digits, special chars) for user creation and password reset. Keep login min_length=1 to avoid leaking whether the password was partially correct.
|
|
|
|
### M-3: F-String Interpolation of Table/Column Names in Raw SQL
|
|
**File:** `app/plugins/builtins/unified_search/embedding.py:194`, `search_engine.py:153`, `routes.py:294,300`, `jobs.py:183,228`
|
|
**Risk:** Multiple raw SQL queries use f-strings to interpolate table and column names: `f"UPDATE {table} SET ..."`, `f"SELECT {emb_col} FROM {table_name} ..."`. While the values come from hardcoded `table_map` dicts (not user input), this pattern is fragile — a future change could introduce user-controlled values into the map.
|
|
**Remediation:** Use SQLAlchemy ORM queries instead of raw SQL where possible. If raw SQL is needed, validate table/column names against an allowlist before interpolation, or use `sqlalchemy.sql.quoted_name` for safe identifier quoting.
|
|
|
|
### M-4: Forgejo Error Reporter Sends Full Context to External Service
|
|
**File:** `app/plugins/builtins/forgejo_error_reporter/service.py:196-199`
|
|
**Risk:** The error reporter serializes the entire `context` dict into the Forgejo issue body as JSON. If frontend error reporting includes sensitive data (user tokens, PII, tenant data), it will be written to an external Forgejo repository as a public issue.
|
|
**Remediation:** Add a field-level allowlist for context data. Strip or redact sensitive keys (tokens, passwords, emails, phone numbers). Consider making Forgejo issues private/confidential.
|
|
|
|
### M-5: Config Has Hardcoded Default Secret Key
|
|
**File:** `app/config.py:55`
|
|
**Risk:** The default `secret_key = "change-me-in-production-use-a-secure-random-string"` is a known public value. While production mode checks for it, development mode silently uses it. If dev environments are exposed (even temporarily), all signed tokens are forgeable.
|
|
**Remediation:** Remove the default value entirely. Make `secret_key` a required field with no default. Fail in all environments if not set.
|
|
|
|
### M-6: `LocalStorage.get_url()` Returns Absolute Filesystem Path
|
|
**File:** `app/core/storage.py:116-117`
|
|
**Risk:** `get_url()` returns `self._full_path(path)` which is the absolute filesystem path (e.g., `/data/uploads/tenant1/file.pdf`). If this URL is returned to the frontend or used in API responses, it leaks the internal directory structure and can aid path traversal attacks.
|
|
**Remediation:** Return a relative path or a signed download URL that routes through an authenticated API endpoint.
|
|
|
|
### M-7: Inconsistent Environment Configuration in `.env`
|
|
**File:** `.env:3,4`
|
|
**Risk:** `.env` sets `ENVIRONMENT=development` but `SESSION_COOKIE_SECURE=true`. In development with HTTP, secure cookies won't be sent, causing auth failures. More importantly, the `ENVIRONMENT=development` setting disables the production safety checks in `get_settings()`, allowing the default `SECRET_KEY` to be used.
|
|
**Remediation:** Use separate `.env.development` and `.env.production` files. Ensure development configs are never accidentally deployed.
|
|
|
|
### M-8: Permission Cache Falls Back to Stale Data on DB Error
|
|
**File:** `app/core/permissions.py:337-344`
|
|
**Risk:** When `_get_current_permission_version()` fails (DB error), the code sets `current_version = cached_version` and uses potentially stale cached permissions. If a user's permissions were revoked during the DB outage, they retain elevated access.
|
|
**Remediation:** On DB error, either fail closed (deny access) or use a shorter stale-while-error TTL. Log the event as a security incident.
|
|
|
|
---
|
|
|
|
## LOW Issues
|
|
|
|
### L-1: `document.write()` with DOM Clone in Print Utility
|
|
**File:** `frontend/src/utils/print.ts:54, 127`
|
|
**Risk:** `printElement()` and `exportToPDF()` use `document.write()` with `clone.outerHTML`. If the printed DOM element contains user-controlled content (e.g., contact notes with HTML), it executes in a new window context. The new window is same-origin, limiting the impact, but it's still an unnecessary risk.
|
|
**Remediation:** Use DOM APIs (`appendChild`, `importNode`) instead of `document.write()`. Alternatively, sanitize the cloned HTML before writing.
|
|
|
|
### L-2: Session Data Stored in Redis Without Encryption
|
|
**File:** `app/core/auth.py:130-134`
|
|
**Risk:** Session data (user_id, tenant_id, email, role, csrf_token, is_system_admin) is stored as plaintext JSON in Redis. Anyone with Redis access can read all active sessions.
|
|
**Remediation:** Encrypt session data before storing in Redis, or accept the risk given Redis should be network-isolated. At minimum, ensure Redis requires authentication and is not exposed.
|
|
|
|
### L-3: No Security Headers Middleware
|
|
**File:** No security headers middleware found
|
|
**Risk:** The application does not set security headers like `X-Content-Type-Options`, `X-Frame-Options`, `Strict-Transport-Security`, `Content-Security-Policy`.
|
|
**Remediation:** Add a security headers middleware or use `starlette-securehead`/`secure` package.
|
|
|
|
### L-4: No Origin Verification on WebSocket Upgrade
|
|
**File:** Both WebSocket managers
|
|
**Risk:** Neither WebSocket manager checks the `Origin` header before accepting connections. While cookies with `SameSite=Strict` provide some protection, some browsers and non-browser clients may not respect SameSite on WebSocket connections.
|
|
**Remediation:** Check `websocket.headers.get("origin")` against `settings.cors_origin_list` before calling `websocket.accept()`.
|
|
|
|
### L-5: Unbounded Feedback/Command Storage in AI UI Control WebSocket
|
|
**File:** `app/plugins/builtins/ai_ui_control/websocket_manager.py:94-103`
|
|
**Risk:** `store_feedback()` stores feedback dicts without size limits. `cleanup_stale()` only runs when explicitly called. An attacker who can send WebSocket messages could fill memory with large feedback payloads.
|
|
**Remediation:** Add size limits on feedback payloads. Run `cleanup_stale()` on a timer or on each `connect()`/`disconnect()`.
|
|
|
|
---
|
|
|
|
## Positive Findings
|
|
|
|
1. **Dockerfile security:** Multi-stage build, non-root user (`appuser` UID 1000), healthcheck configured, no secrets baked into image.
|
|
2. **RLS implementation:** PostgreSQL Row Level Security with `FORCE` (migration 0028) ensures tenant isolation even for table owners. `set_tenant_context()` uses parameterized queries.
|
|
3. **Password hashing:** bcrypt with configurable rounds (default 12).
|
|
4. **Session tokens:** `secrets.token_urlsafe(32)` — cryptographically secure.
|
|
5. **XSS protection:** `HtmlBlock.tsx` and `SignatureManager.tsx` use `DOMPurify.sanitize()` before `dangerouslySetInnerHTML`.
|
|
6. **RBAC architecture:** Deny-list takes precedence over allow-list. Field-level permissions with strictest-wins merging. Permission version-based cache invalidation.
|
|
7. **No user enumeration:** Password reset endpoint always returns 200.
|
|
8. **SQL injection:** ORM queries use parameterized statements throughout. Raw SQL in `unified_search` uses hardcoded maps (not directly exploitable).
|
|
9. **`.gitignore`** properly covers `.env`, `.env.*`, and excludes example files.
|
|
10. **Production safety checks** in `get_settings()` validate `SECRET_KEY`, `SESSION_COOKIE_SECURE`, and `STORAGE_PATH`.
|
|
|
|
---
|
|
|
|
## Migration & Data Loss Risks
|
|
|
|
1. **RLS policies:** Multiple migrations (0001, 0002, 0004, 0015, 0021, 0028) create and modify RLS policies. Migration 0028 adds `FORCE ROW LEVEL SECURITY`. Ensure all migrations are applied in order before production deployment.
|
|
2. **Backup risk:** No backup/restore procedure found in the repository. The `last_backup_at` system setting is referenced in automation jobs but no backup script exists.
|
|
3. **Volume persistence:** `docker-compose.yml` defines named volumes for `pgdata`, `redisdata`, and `storage`. Good for persistence, but no backup strategy documented.
|
|
4. **Migration rollback:** Down migrations exist but should be tested. RLS policy down migrations disable RLS — running a rollback in production would expose all tenant data.
|
|
|
|
---
|
|
|
|
## Remediation Priority
|
|
|
|
1. **Immediate (before any production deploy):** C-1, C-2, C-3, C-4, C-5, H-1, H-2, H-3
|
|
2. **Short-term (within 1 sprint):** H-4, H-5, H-6, H-7, H-8, M-1, M-2, M-5
|
|
3. **Medium-term (within 2 sprints):** M-3, M-4, M-6, M-7, M-8, L-1, L-2, L-3, L-4, L-5
|