docs(phase-06): add pattern map and code review fix report
06-PATTERNS.md: maps 12 new/modified files to closest codebase analogs for Phase 6 (Performance & Production Hardening). 06-REVIEW-FIX.md: records all 16 review findings (7 critical + 9 warning) fixed in iteration 1 on 2026-06-04. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
curo1305
co-authored by
Claude Sonnet 4.6
Claude Sonnet 4.6
parent
1fd7395893
commit
18de84d1a9
@@ -0,0 +1,163 @@
|
||||
---
|
||||
phase: 06-performance-production-hardening
|
||||
fixed_at: 2026-06-04T00:00:00Z
|
||||
review_path: .planning/phases/06-performance-production-hardening/06-REVIEW.md
|
||||
iteration: 1
|
||||
findings_in_scope: 16
|
||||
fixed: 16
|
||||
skipped: 0
|
||||
status: all_fixed
|
||||
---
|
||||
|
||||
# Phase 6: Code Review Fix Report
|
||||
|
||||
**Fixed at:** 2026-06-04
|
||||
**Source review:** `.planning/phases/06-performance-production-hardening/06-REVIEW.md`
|
||||
**Iteration:** 1
|
||||
|
||||
**Summary:**
|
||||
- Findings in scope: 16 (7 Critical + 9 Warning)
|
||||
- Fixed: 16
|
||||
- Skipped: 0
|
||||
|
||||
## Fixed Issues
|
||||
|
||||
### CR-01: CLOUD_CREDS_KEY never passed to backend service
|
||||
|
||||
**Files modified:** `docker-compose.yml`
|
||||
**Commit:** a8dbb02
|
||||
**Applied fix:** Added `- CLOUD_CREDS_KEY=${CLOUD_CREDS_KEY}` to the backend service environment block, alongside WR-02, WR-05, WR-06, WR-08.
|
||||
|
||||
---
|
||||
|
||||
### CR-02: Grafana exposed with unauthenticated Admin-role access
|
||||
|
||||
**Files modified:** `docker-compose.yml`
|
||||
**Commit:** a8dbb02
|
||||
**Applied fix:** Changed Grafana env to disable anonymous access and use `${GRAFANA_ADMIN_USER:-admin}` / `${GRAFANA_ADMIN_PASSWORD:-changeme}` credentials. Changed port bindings for both Grafana (`3000:3000`) and Loki (`3100:3100`) to loopback-only (`127.0.0.1:3000:3000` and `127.0.0.1:3100:3100`).
|
||||
|
||||
---
|
||||
|
||||
### CR-03: `get_client_ip()` bypassed — raw `X-Forwarded-For` reads
|
||||
|
||||
**Files modified:** `backend/api/cloud.py`, `backend/api/documents.py`
|
||||
**Commit:** 23c27ef
|
||||
**Applied fix:** Added `from deps.utils import get_client_ip` import to both files. Replaced all five raw `request.headers.get("X-Forwarded-For")` reads (cloud.py lines 629, 766; documents.py lines 276, 384, 670) with `get_client_ip(request)`. Also removed the "TRUST BOUNDARY" comments that noted the problem without fixing it.
|
||||
|
||||
---
|
||||
|
||||
### CR-04: `default_storage_backend` written to DB without allowlist validation
|
||||
|
||||
**Files modified:** `backend/api/cloud.py`
|
||||
**Commit:** 3a6251c
|
||||
**Applied fix:** Added `_VALID_BACKENDS = frozenset({"minio", "google_drive", "onedrive", "nextcloud", "webdav"})` module constant and validation block in `update_default_storage()` that raises HTTP 422 for any value not in the allowlist.
|
||||
|
||||
---
|
||||
|
||||
### CR-05: Audit log leaks attempted email (PII) in `metadata_`
|
||||
|
||||
**Files modified:** `backend/api/auth.py`, `frontend/src/components/admin/AuditLogTab.vue`
|
||||
**Commit:** aad7635
|
||||
**Applied fix:** Added `import hashlib` to auth.py and replaced `{"attempted_email": str(body.email)}` with `{"attempted_email_hash": hashlib.sha256(str(body.email).encode()).hexdigest()[:16]}` in the login failure audit log call. Updated AuditLogTab.vue to display `entry.metadata_.attempted_email_hash` (with `hash:` prefix and monospace styling) instead of `entry.metadata_.attempted_email`.
|
||||
|
||||
---
|
||||
|
||||
### CR-06: `CorrelationIDMiddleware` binds `duration_ms` after response is delivered — value never logged
|
||||
|
||||
**Files modified:** `backend/main.py`
|
||||
**Commit:** a37a910
|
||||
**Applied fix:** Added `_response_status: int = 0` variable and `nonlocal _response_status` capture in the `send_with_header` closure to record the HTTP status code. After `await self.app(...)` computes `duration_ms`, now emits a structured log line via `structlog.get_logger("docuvault.access").info("request_complete", status_code=_response_status)` so `duration_ms` is written to the log before context is cleared.
|
||||
|
||||
---
|
||||
|
||||
### CR-07: `event_type` LIKE filter allows unvalidated user input with SQL wildcards
|
||||
|
||||
**Files modified:** `backend/api/audit.py`, `backend/tests/test_audit.py`
|
||||
**Commit:** 10970d9 (fix), fb4ce29 (test update)
|
||||
**Applied fix:** Added `_VALID_EVENT_PREFIXES = frozenset({"auth", "document", "folder", "share", "admin", "cloud"})` module constant. Added validation before each of the three `.like()` call sites (`_build_filtered_query`, `_build_filtered_query_with_handles`, and the inline count query in `list_audit_log`). Changed LIKE pattern from `f"{event_type}%"` to `f"{event_type}.%"` to enforce true prefix semantics. Updated `test_audit_log_filter_by_event_type` to pass `"document"` prefix instead of the full `"document.uploaded"` event type string.
|
||||
|
||||
---
|
||||
|
||||
### WR-01: `auth_limiter` not reset between tests
|
||||
|
||||
**Files modified:** `backend/tests/conftest.py`
|
||||
**Commit:** 4a57193
|
||||
**Applied fix:** Added `from api.auth import limiter as auth_limiter` import and added `auth_limiter._storage.reset()` calls both before and after `yield` in the `reset_rate_limiter` autouse fixture.
|
||||
|
||||
---
|
||||
|
||||
### WR-02: `uvicorn --reload` in docker-compose production backend command
|
||||
|
||||
**Files modified:** `docker-compose.yml`
|
||||
**Commit:** a8dbb02
|
||||
**Applied fix:** Changed `command: uvicorn main:app --host 0.0.0.0 --port 8000 --reload` to `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
|
||||
|
||||
**Files modified:** `backend/load_tests/locustfile.py`
|
||||
**Commit:** 013802a
|
||||
**Applied fix:** Changed `docs = resp.json()` to `docs = resp.json().get("items", [])` in the `get_document` task so it correctly handles the `{"items": [...], "total": N, ...}` response envelope.
|
||||
|
||||
---
|
||||
|
||||
### WR-04: `trusted_proxy` list missing `10.0.0.0/8`
|
||||
|
||||
**Files modified:** `backend/deps/utils.py`
|
||||
**Commit:** b0d2406
|
||||
**Applied fix:** Added `ipaddress.ip_network("10.0.0.0/8")` as the first entry in `_TRUSTED_PROXY_NETS`, covering cloud VPC, Kubernetes pod CIDRs, and custom Docker network configurations that use the 10.x.x.x range.
|
||||
|
||||
---
|
||||
|
||||
### WR-05: `celery-beat` service lacks container hardening
|
||||
|
||||
**Files modified:** `docker-compose.yml`
|
||||
**Commit:** a8dbb02
|
||||
**Applied fix:** Added `read_only: true`, `tmpfs: ["/tmp:mode=1777"]`, `cap_drop: [ALL]`, and `security_opt: ["no-new-privileges:true"]` to the celery-beat service. Changed the `command` to pass `--schedule /tmp/celerybeat-schedule` so the schedule file goes to the tmpfs mount instead of the read-only root filesystem. Removed the "NOT hardened" comment.
|
||||
|
||||
---
|
||||
|
||||
### WR-06: `LOG_JSON` hardcoded to `true` in docker-compose
|
||||
|
||||
**Files modified:** `docker-compose.yml`
|
||||
**Commit:** a8dbb02
|
||||
**Applied fix:** Changed `- LOG_JSON=${LOG_JSON:-false}` (was `- LOG_JSON=true #${LOG_JSON:-false}` in working tree) to `- LOG_JSON=${LOG_JSON:-true}` — defaults to `true` in production but allows developer override via `.env`. The default changed from `false` to `true` to match production logging intent.
|
||||
|
||||
---
|
||||
|
||||
### WR-07: `print()` used for cloud delete errors instead of structlog
|
||||
|
||||
**Files modified:** `backend/api/documents.py`
|
||||
**Commit:** 7cd29e9
|
||||
**Applied fix:** Added `import structlog as _structlog` and `_log = _structlog.get_logger(__name__)` at module level. Replaced `import sys; print(f"[cloud-delete] provider error: {exc}", file=sys.stderr)` with `_log.warning("cloud_delete_failed", provider=doc.storage_backend, error=str(exc))`.
|
||||
|
||||
---
|
||||
|
||||
### WR-08: `celery-worker` missing `SECRET_KEY`
|
||||
|
||||
**Files modified:** `docker-compose.yml`
|
||||
**Commit:** a8dbb02
|
||||
**Applied fix:** Added `- SECRET_KEY=${SECRET_KEY}` to the celery-worker service environment block.
|
||||
|
||||
---
|
||||
|
||||
### WR-09: `AuditLogTab.vue` silently swallows fetch errors — no user feedback
|
||||
|
||||
**Files modified:** `frontend/src/components/admin/AuditLogTab.vue`
|
||||
**Commit:** 21366bd
|
||||
**Applied fix:** Added `const fetchError = ref(null)` reactive ref. Set `fetchError.value = null` at the start of `fetchLog()` and `fetchError.value = 'Failed to load audit log. Please try again.'` in the catch block. Added `<p v-else-if="fetchError" class="text-xs text-red-600 mt-1">{{ fetchError }}</p>` between the loading state and empty state elements in the template.
|
||||
|
||||
---
|
||||
|
||||
## Test Results
|
||||
|
||||
Backend test suite run after all fixes:
|
||||
- **366 passed**, 1 failed (pre-existing `test_extract_docx` — `ModuleNotFoundError: No module named 'docx'` in local dev environment, unrelated to these fixes), 6 skipped, 12 xfailed.
|
||||
- The `test_audit_log_filter_by_event_type` failure from the CR-07 fix was resolved by updating the test to use the prefix-based filter API.
|
||||
|
||||
---
|
||||
|
||||
_Fixed: 2026-06-04_
|
||||
_Fixer: Claude (gsd-code-fixer)_
|
||||
_Iteration: 1_
|
||||
Reference in New Issue
Block a user