diff --git a/.planning/phases/06-performance-production-hardening/06-REVIEW.md b/.planning/phases/06-performance-production-hardening/06-REVIEW.md new file mode 100644 index 0000000..35c2caf --- /dev/null +++ b/.planning/phases/06-performance-production-hardening/06-REVIEW.md @@ -0,0 +1,525 @@ +--- +phase: 06-performance-production-hardening +reviewed: 2026-06-04T00:00:00Z +depth: standard +files_reviewed: 24 +files_reviewed_list: + - backend/Dockerfile + - backend/api/audit.py + - backend/api/auth.py + - backend/api/cloud.py + - backend/api/documents.py + - backend/api/shares.py + - backend/config.py + - backend/deps/utils.py + - backend/load_tests/locustfile.py + - backend/main.py + - backend/services/logging.py + - backend/services/rate_limiting.py + - backend/tests/conftest.py + - backend/tests/test_audit.py + - backend/tests/test_logging.py + - backend/tests/test_rate_limiting.py + - docker-compose.yml + - docker/loki/loki-config.yaml + - docker/loki/promtail-config.yaml + - frontend/src/components/admin/AuditLogTab.vue + - frontend/src/components/sharing/ShareModal.vue + - frontend/src/stores/documents.js + - frontend/src/views/AccountView.vue + - frontend/src/views/FileManagerView.vue +findings: + critical: 7 + warning: 9 + info: 4 + total: 20 +status: issues_found +--- + +# Phase 6: Code Review Report + +**Reviewed:** 2026-06-04 +**Depth:** standard +**Files Reviewed:** 24 +**Status:** issues_found + +## Summary + +Phase 6 added structured logging (structlog + CorrelationIDMiddleware), per-account rate limiting +(slowapi), container hardening (read_only/tmpfs/cap_drop), the audit log viewer/export UI, and share +permission editing. The core middleware implementation is solid. The majority of defects are in +cross-cutting concerns: `get_client_ip` is bypassed in several routers, the +`CLOUD_CREDS_KEY` secret is missing from the backend service in docker-compose, Grafana is +exposed with unauthenticated Admin access, and the locust load test accesses document list results +in a shape that does not match the actual API response envelope. Several warning-tier issues relate +to unvalidated user-supplied values written directly to the database, missing rate-limit resets for +the auth limiter in tests, and the uvicorn `--reload` flag committed for the production container. + +--- + +## Critical Issues + +### CR-01: CLOUD_CREDS_KEY never passed to backend service — falls back to hardcoded default + +**File:** `docker-compose.yml:55-71` +**Issue:** The backend service's `environment:` block does not include `CLOUD_CREDS_KEY`. The +`celery-worker` service at line 101 does pass it, but the FastAPI backend — which encrypts and +decrypts cloud credentials on every OAuth callback, WebDAV connect, folder listing, upload, and +document download — silently falls back to the default value `"CHANGEME-32-bytes-padded!!"` defined +in `config.py:61`. Any cloud credentials stored in production are therefore encrypted with the +publicly-known placeholder key, exposing them to anyone who can read the database. + +**Fix:** Add the missing environment variable to the backend service block: +```yaml + backend: + environment: + ... + - CLOUD_CREDS_KEY=${CLOUD_CREDS_KEY} +``` + +--- + +### CR-02: Grafana exposed with unauthenticated Admin-role access + +**File:** `docker-compose.yml:170-172` +**Issue:** Grafana is configured with `GF_AUTH_ANONYMOUS_ENABLED=true` and +`GF_AUTH_ANONYMOUS_ORG_ROLE=Admin`. Any user able to reach port 3000 on the host has full Grafana +Admin privileges with no credentials. Grafana Admin access includes datasource management, dashboard +modification, and in many versions allows arbitrary HTTP requests to backend services (SSRF via data +source). Loki at port 3100 is also exposed without authentication, allowing unauthenticated read of +all structured logs (which include correlation IDs, paths, and user IDs). + +**Fix:** +```yaml + grafana: + environment: + - GF_AUTH_ANONYMOUS_ENABLED=false + - GF_SECURITY_ADMIN_USER=${GRAFANA_ADMIN_USER} + - GF_SECURITY_ADMIN_PASSWORD=${GRAFANA_ADMIN_PASSWORD} +``` +Additionally, expose Grafana and Loki only on loopback (`127.0.0.1:3000:3000`) or behind the +application reverse proxy with authentication. + +--- + +### CR-03: `get_client_ip()` bypassed — raw `X-Forwarded-For` reads in cloud.py and documents.py + +**File:** `backend/api/cloud.py:629`, `backend/api/cloud.py:766`, `backend/api/documents.py:276`, `backend/api/documents.py:384`, `backend/api/documents.py:670` + +**Issue:** Phase 6 added `get_client_ip()` in `deps/utils.py` with trusted-proxy CIDR validation +as the canonical IP extractor for audit logging. However, five call-sites in `cloud.py` and +`documents.py` read `request.headers.get("X-Forwarded-For")` directly, bypassing the trusted-proxy +check entirely. An external attacker can set any arbitrary string in `X-Forwarded-For` and have it +written verbatim into the audit log. Although the comment in `documents.py` acknowledges the trust +boundary, the correct fix is to call `get_client_ip()` rather than noting the problem and leaving +it unfixed — especially given that the canonical helper was introduced in this same phase. + +**Fix:** Replace every raw `X-Forwarded-For` read with `get_client_ip(request)`: +```python +# backend/api/cloud.py line 629 (connect_webdav), line 766 (delete_connection) +# backend/api/documents.py lines 276, 384, 670 +from deps.utils import get_client_ip # already imported in shares.py +_ip = get_client_ip(request) # replaces the raw header read in every site +``` + +--- + +### CR-04: `default_storage_backend` written to DB without allowlist validation + +**File:** `backend/api/cloud.py:961` +**Issue:** The `PATCH /api/users/me/default-storage` endpoint accepts `body.backend` (a plain +`str`) and writes it directly to `user.default_storage_backend` with no validation against an +allowlist of known providers. The comment says "validated by the frontend dropdown" which is not a +server-side control. An authenticated user can set the field to any arbitrary string. Downstream +code that branches on `default_storage_backend` would receive an unexpected value; combined with +future extensions this is a mass-assignment / logic bypass vector. + +**Fix:** +```python +_VALID_BACKENDS = frozenset({"minio", "google_drive", "onedrive", "nextcloud", "webdav"}) + +@users_router.patch("/me/default-storage") +async def update_default_storage(body: DefaultStorageRequest, ...): + if body.backend not in _VALID_BACKENDS: + raise HTTPException( + status_code=422, + detail=f"Invalid backend. Valid values: {sorted(_VALID_BACKENDS)}", + ) + ... +``` + +--- + +### CR-05: Audit log leaks attempted email (PII) in `metadata_` — surfaced in admin UI + +**File:** `backend/api/auth.py:216`, `frontend/src/components/admin/AuditLogTab.vue:114` + +**Issue:** On login failure, the audit log writes `metadata_={"attempted_email": str(body.email)}`. +The admin audit log viewer at AuditLogTab.vue:114 explicitly reads and displays +`entry.metadata_.attempted_email` as the email column. The audit log export endpoint also writes +`metadata_` as a JSON column in the CSV. CLAUDE.md's security protocol states "all auth events +written to audit log without document content" and "PII fields encrypted at rest." Storing raw +email addresses in `metadata_` (an unencrypted JSONB column) and rendering them in the admin UI is +inconsistent with the PII encryption requirement. This also affects GDPR/CCPA obligations as the +email of a failed-login attempt is retained indefinitely in the audit log. + +**Fix:** At minimum, hash or truncate the email in the metadata before storage: +```python +import hashlib +metadata_={"attempted_email_hash": hashlib.sha256(str(body.email).encode()).hexdigest()[:16]}, +``` +Or omit the email from audit metadata entirely — the user_id (when found) already identifies the +account. If the email must be retained for forensic purposes it must be encrypted with the same +per-row HKDF key used for user PII. + +--- + +### CR-06: `CorrelationIDMiddleware` binds `duration_ms` after the response is already delivered — value is never logged + +**File:** `backend/main.py:119-123` + +**Issue:** The middleware calls `await self.app(scope, receive, send_with_header)` which yields +control only after the full response has been sent to the client. The `duration_ms` binding at +lines 122-123 runs after the response is complete. Any log statements emitted during the request +handler already ran before `duration_ms` was bound, so no log line actually sees this field. The +docstring at line 89 claims "After response: bind duration_ms for final log emission" — but +`CorrelationIDMiddleware` emits no log line itself (it only binds to contextvars), so +`duration_ms` is computed and bound to a context that is about to be cleared by the next request's +`clear_contextvars()`. The metric is silently discarded on every request. + +**Fix:** Emit a structured log line from within the middleware after binding `duration_ms`, or +move the timing to `send_with_header` where it can be attached to the `http.response.start` event: +```python + await self.app(scope, receive, send_with_header) + duration_ms = (time.perf_counter_ns() - start_ns) / 1_000_000 + structlog.contextvars.bind_contextvars(duration_ms=round(duration_ms, 2)) + structlog.get_logger("docuvault.access").info( + "request_complete", + status_code=_response_status, # capture in send_with_header closure + ) +``` + +--- + +### CR-07: `event_type` LIKE filter allows unvalidated user input with SQL wildcards + +**File:** `backend/api/audit.py:124`, `backend/api/audit.py:164`, `backend/api/audit.py:291` + +**Issue:** The `event_type` query parameter is interpolated directly into a SQLAlchemy +`like(f"{event_type}%")` call at three locations. While SQLAlchemy parameterises the bind +value (preventing SQL injection), the value itself is never validated against an allowlist of +known event-type prefixes. An admin could pass `event_type=%` (matching all rows) or +`event_type=____` (single-char wildcard patterns) to extract data in ways not intended by the +filter interface. More importantly, a `%` in the middle of the value bypasses the prefix-match +semantics the API documents. + +**Fix:** Validate `event_type` against the known prefix set before use: +```python +_VALID_EVENT_PREFIXES = frozenset({"auth", "document", "folder", "share", "admin", "cloud"}) + +if event_type is not None: + if event_type not in _VALID_EVENT_PREFIXES: + raise HTTPException(status_code=422, detail="Invalid event_type prefix") + q = q.where(AuditLog.event_type.like(f"{event_type}.%")) +``` + +--- + +## Warnings + +### WR-01: `auth_limiter` (IP-level) not reset between tests — cross-test contamination + +**File:** `backend/tests/conftest.py:160-171` + +**Issue:** The `reset_rate_limiter` autouse fixture resets `account_limiter._storage` (the +per-user limiter) but does not reset `auth_limiter._storage` (the IP-level limiter from +`api/auth.py`). Test suites that call `/api/auth/login`, `/api/auth/register`, or +`/api/auth/refresh` in a tight loop can hit the IP-level 10 req/minute limit in a later test, +causing spurious 429 failures that are hard to diagnose. `test_rate_limiting.py` already tests +the account limiter in isolation via a separate `_isolated_limiter`, but the shared `auth_limiter` +module singleton is never cleared. + +**Fix:** +```python +@pytest.fixture(autouse=True) +def reset_rate_limiter(): + from services.rate_limiting import account_limiter + from api.auth import limiter as auth_limiter + account_limiter._storage.reset() + auth_limiter._storage.reset() + yield + account_limiter._storage.reset() + auth_limiter._storage.reset() +``` + +--- + +### WR-02: `uvicorn --reload` in docker-compose production backend command + +**File:** `docker-compose.yml:76` + +**Issue:** The backend service starts with `uvicorn main:app --host 0.0.0.0 --port 8000 --reload`. +`--reload` enables file-system watching and triggers automatic restarts on code changes. In a +container with `volumes: - ./backend:/app`, this means any local developer file-system change +immediately restarts the production process. Beyond the stability risk, `--reload` mode starts +additional reloader threads that can interfere with the read-only filesystem constraint (it tries +to watch inotify), and it disables uvicorn's built-in worker process isolation. The Dockerfile +CMD at line 36 correctly omits `--reload`, so this is a docker-compose override problem. + +**Fix:** +```yaml + command: uvicorn main:app --host 0.0.0.0 --port 8000 --workers 2 +``` + +--- + +### WR-03: Locust load test accesses document list as a bare list — shape mismatch + +**File:** `backend/load_tests/locustfile.py:78-82` + +**Issue:** `get_document()` calls `GET /api/documents/` and then accesses the result as: +```python +docs = resp.json() +if docs: + doc_id = docs[0]["id"] +``` +The actual API response is `{"items": [...], "total": N, "page": 1, "per_page": 20}` (an object, +not a list). `resp.json()` returns a dict, which is truthy even when `items` is empty, so +`docs[0]["id"]` will raise a `TypeError` (`dict indices must be integers`) every time the +`get_document` task runs. The task silently suppresses the error (locust catches all exceptions), +producing misleading "successful" request counts that mask actual 500-class errors during load runs. + +**Fix:** +```python +docs = resp.json().get("items", []) +if docs: + doc_id = docs[0]["id"] +``` + +--- + +### WR-04: `trusted_proxy` list missing `10.0.0.0/8` — Docker networks excluded + +**File:** `backend/deps/utils.py:10-15` + +**Issue:** The `_TRUSTED_PROXY_NETS` list covers `127.0.0.0/8`, `172.16.0.0/12`, and +`192.168.0.0/16` but omits `10.0.0.0/8`. Docker's default bridge network assigns addresses in +the `172.17.0.0/16` range (covered), but Docker Compose networks default to `172.18.0.0/16` +through `172.31.0.0/16` (also covered by `172.16.0.0/12`). However, some deployments — including +cloud VPCs, Kubernetes pod CIDRs, and custom Docker network configurations — use the `10.0.0.0/8` +block. In those environments the reverse proxy (nginx/traefik) sits on a 10.x.x.x address, the +CIDR check fails, and `X-Forwarded-For` is silently ignored in favour of the proxy's own IP, +logging all requests as originating from the proxy itself. + +**Fix:** +```python +_TRUSTED_PROXY_NETS = [ + ipaddress.ip_network("10.0.0.0/8"), # add this + ipaddress.ip_network("127.0.0.0/8"), + ipaddress.ip_network("172.16.0.0/12"), + ipaddress.ip_network("192.168.0.0/16"), + ipaddress.ip_network("::1/128"), +] +``` + +--- + +### WR-05: `celery-beat` service lacks all container hardening present on other workers + +**File:** `docker-compose.yml:125-145` + +**Issue:** Phase 6 added `read_only: true`, `tmpfs`, `cap_drop: ALL`, and +`security_opt: no-new-privileges` to the `backend` and `celery-worker` services. The `celery-beat` +service at lines 125-145 has none of these controls. The comment "NOT hardened — writes +celerybeat-schedule to working directory" explains the intent but `celerybeat-schedule` is a small +file that could be redirected to `/tmp`. Leaving `celery-beat` without `cap_drop` and +`no-new-privileges` is an unnecessary surface area — it runs the same image as the worker. + +**Fix:** Add a tmpfs mount for the schedule file and apply identical hardening: +```yaml + celery-beat: + ... + command: celery -A celery_app beat --loglevel=info --schedule /tmp/celerybeat-schedule + read_only: true + tmpfs: + - "/tmp:mode=1777" + cap_drop: + - ALL + security_opt: + - "no-new-privileges:true" +``` + +--- + +### WR-06: `LOG_JSON` hardcoded to `true` in docker-compose, env-var override silently ignored + +**File:** `docker-compose.yml:71` + +**Issue:** Line 71 reads `- LOG_JSON=true #${LOG_JSON:-false}`. The env-var interpolation is +commented out and the literal `true` is always passed. A developer who sets `LOG_JSON=false` in +their `.env` file for human-readable output will not see the effect because the compose file +overrides it unconditionally. This is a maintenance hazard. + +**Fix:** +```yaml + - LOG_JSON=${LOG_JSON:-true} +``` + +--- + +### WR-07: `print()` used for cloud delete errors instead of structured logger + +**File:** `backend/api/documents.py:678-679` + +**Issue:** When a cloud provider delete fails in `delete_document()`, the error is written via +`print(f"[cloud-delete] provider error: {exc}", file=sys.stderr)`. This bypasses structlog +entirely: the line will not carry a correlation ID, will not be picked up by promtail (which reads +structured JSON), and will not appear in Loki. Phase 6's explicit goal was to route all logging +through structlog. + +**Fix:** +```python +import structlog as _structlog +_log = _structlog.get_logger(__name__) +_log.warning("cloud_delete_failed", provider=doc.storage_backend, error=str(exc)) +``` + +--- + +### WR-08: `celery-worker` missing `SECRET_KEY` — JWT validation fails for task-triggered operations + +**File:** `docker-compose.yml:92-123` + +**Issue:** The `celery-worker` service environment block does not include `SECRET_KEY`. If any +Celery task validates JWTs (e.g., tasks triggered by authenticated user actions that re-use the +auth context), the worker will use the default `"CHANGEME"` key from `config.py:31`, which is +different from the production `SECRET_KEY`. This causes silent token validation failures or, +worse, creates a second valid signing key if the production key has been set. Similarly, +`DATABASE_MIGRATE_URL` is absent from the worker, which is acceptable unless the worker runs +migrations, but `SECRET_KEY` omission is an active risk. + +**Fix:** Add to `celery-worker` environment: +```yaml + - SECRET_KEY=${SECRET_KEY} +``` + +--- + +### WR-09: `AuditLogTab.vue` silently swallows fetch errors — no user feedback + +**File:** `frontend/src/components/admin/AuditLogTab.vue:234-238` + +**Issue:** The `fetchLog()` function catches all exceptions with an empty handler: +```js + } catch (e) { + entries.value = [] + } +``` +No error message is shown to the admin user. If the audit log API returns a network error or 5xx, +the UI displays "No audit log entries match the selected filters" — indistinguishable from a +legitimately empty result. An admin has no signal that the log viewer is broken. + +**Fix:** +```js +const fetchError = ref(null) +// ... + } catch (e) { + entries.value = [] + fetchError.value = 'Failed to load audit log. Please try again.' + } +``` +And add `

{{ fetchError }}

` to the template. + +--- + +## Info + +### IN-01: `Dockerfile` does not pin base image by digest + +**File:** `backend/Dockerfile:1` and `backend/Dockerfile:14` + +**Issue:** Both stages use `python:3.12-slim` without a digest pin (e.g. +`python:3.12-slim@sha256:...`). If the upstream image is silently updated or compromised, the next +`docker build` will pull the new image with no warning. For a security-critical service this is a +supply-chain risk. CLAUDE.md requires "dependency pinning ... no floating >= for security-critical +packages." + +**Fix:** Pin by digest after testing: +```dockerfile +FROM python:3.12-slim@sha256: AS builder +``` + +--- + +### IN-02: `revokeShare` and `listShares` in documents store are trivial pass-throughs + +**File:** `frontend/src/stores/documents.js:166-176` + +**Issue:** Three functions (`revokeShare`, `listShares`, `updateSharePermission`) consist only of +`try { return await api.X() } catch (e) { throw e }` — they catch and immediately re-throw the +exception without adding any value. The catch block is dead code that adds stack trace noise. + +**Fix:** Remove the try/catch wrappers: +```js +async function revokeShare(shareId) { + await api.deleteShare(shareId) +} +async function listShares(docId) { + return api.listShares(docId) +} +async function updateSharePermission(shareId, permission) { + return api.updateSharePermission(shareId, permission) +} +``` + +--- + +### IN-03: `locustfile.py` uses a single shared `TEST_HANDLE` across all virtual users + +**File:** `backend/load_tests/locustfile.py:31`, `backend/load_tests/locustfile.py:52-54` + +**Issue:** All 50 simulated users attempt to register with `handle="loadtestuser"`. The first user +succeeds; all subsequent registrations receive 409. The `on_start` ignores the registration +response entirely (no `name=` parameter, response not checked), so the 409s are counted as errors +in Locust stats unless the `name=` parameter is set. Beyond the stat noise, all virtual users +share the same account, meaning the per-account rate limiter (100 req/minute) will trigger well +before the intended 50-user load is exercised. + +**Fix:** Make the handle per-user: +```python +import random, string +TEST_HANDLE = f"loadtest_{''.join(random.choices(string.ascii_lowercase, k=8))}" +``` +Or use the user_id of the Locust `HttpUser` instance. + +--- + +### IN-04: `handleFolderRename` swallows all errors silently + +**File:** `frontend/src/views/FileManagerView.vue:128-131` + +**Issue:** +```js +async function handleFolderRename({ id, name }) { + if (!name) return + try { await foldersStore.renameFolder(id, name) } catch {} +} +``` +All errors are swallowed with an empty catch. If the rename API call fails (network error, +duplicate name, 403), the UI shows no feedback — the folder appears to rename locally but reverts +on the next fetch without explanation. Compare with `handleFolderCreate` which calls `onError`. + +**Fix:** Emit an error event or use the store error field: +```js +async function handleFolderRename({ id, name, onError }) { + if (!name) return + try { await foldersStore.renameFolder(id, name) } + catch (e) { onError?.(e.message || 'Rename failed.') } +} +``` + +--- + +_Reviewed: 2026-06-04_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_