21 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 06-performance-production-hardening | 2026-06-04T00:00:00Z | standard | 24 |
|
|
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:
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:
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):
# 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:
_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:
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:
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:
_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:
@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:
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:
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:
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:
_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:
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:
- 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:
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:
- 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:
} 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:
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:
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:
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:
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:
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:
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