curo/kite
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(06): add code review report — 7 critical, 9 warning, 4 info findings

Browse Source 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
curo1305
2026-06-04 23:01:53 +02:00
co-authored by Claude Sonnet 4.6
parent b8b0840729
commit 888856aa8b
1 changed files with 525 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/06-performance-production-hardening/06-REVIEW.md
+525
View File
@@ -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 `<p v-if="fetchError" class="text-xs text-red-600">{{ fetchError }}</p>` 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:<verified-digest> 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_