docs(12): add gap closure code review report

2026-06-20 10:54:22 +02:00
parent bb818e0621
commit b6526e46f1
1 changed files with 322 additions and 0 deletions
@@ -0,0 +1,322 @@
+---
+phase: 12-cloud-resource-foundation
+reviewed: 2026-06-20T00:00:00Z
+depth: standard
+files_reviewed: 10
+files_reviewed_list:
+  - docker-compose.yml
+  - backend/tests/test_compose_migrations.py
+  - backend/tests/test_migration_0006.py
+  - backend/main.py
+  - frontend/package.json
+  - AGENTS.md
+  - README.md
+  - RUNBOOK.md
+  - SECURITY.md
+  - CLAUDE.md
+findings:
+  critical: 3
+  warning: 5
+  info: 3
+  total: 11
+status: issues_found
+---
+
+# Phase 12: Code Review Report
+
+**Reviewed:** 2026-06-20
+**Depth:** standard
+**Files Reviewed:** 10
+**Status:** issues_found
+
+## Summary
+
+The Phase 12 gap-closure implementation adds a one-shot `migrate` service to docker-compose.yml,
+seven static compose-invariant tests, and a 12-test integration suite for the 0005→0006 Alembic
+migration. The compose hardening approach (read_only, cap_drop ALL, no-new-privileges, restart: no)
+is correct in concept. However, several concrete defects were found:
+
+- Three critical issues: the `promtail` service mounts the Docker socket with no restrictions (full
+  container escape vector), the Grafana admin password has a hardcoded default with no boot-time
+  override enforcement, and the `test_docuvault_app_cannot_create_table` test silently skips rather
+  than failing when the role is absent — defeating the purpose of the DDL-privilege gate.
+- Five warnings: the `migrate` service mounts the entire source tree as a writable volume (defeating
+  `read_only`), the `minio-init` service has no hardening at all, version-pinned `latest` tags on
+  three images, the integration test class relies on execution order without `@pytest.mark.order`,
+  and `celery-beat` is missing `CLOUD_CREDS_KEY` and JWT key env vars.
+- Three info items: README/AGENTS.md version skew, the `backend` service unnecessarily receives
+  `DATABASE_MIGRATE_URL`, and the health endpoint leaks Python exception class names to callers.
+
+---
+
+## Critical Issues
+
+### CR-01: promtail mounts Docker socket — full container escape
+
+**File:** `docker-compose.yml:259`
+**Issue:** The `promtail` service bind-mounts `/var/run/docker.sock` into the container with no
+read-only flag and no capability restriction. Docker socket access is equivalent to unrestricted
+root on the host: any process inside promtail that is compromised (or any malicious log entry that
+achieves code execution) can spawn privileged containers, exfiltrate secrets from all other
+containers, or escape to the host entirely.
+```yaml
+volumes:
+  - /var/run/docker.sock:/var/run/docker.sock   # no :ro, no hardening
+```
+**Fix:** Mount the socket read-only and add container hardening:
+```yaml
+promtail:
+  image: grafana/promtail:2.9.10   # pin version (see WR-02)
+  volumes:
+    - ./docker/loki/promtail-config.yaml:/etc/promtail/config.yaml
+    - /var/lib/docker/containers:/var/lib/docker/containers:ro
+    - /var/run/docker.sock:/var/run/docker.sock:ro
+  cap_drop:
+    - ALL
+  security_opt:
+    - "no-new-privileges:true"
+  read_only: true
+  tmpfs:
+    - "/tmp:mode=1777"
+```
+Even with `:ro` the socket still grants read access to container metadata (env vars, image names).
+For production, prefer the promtail `file`-based log scraping mode and remove the socket entirely.
+
+---
+
+### CR-02: Grafana admin password has hardcoded `changeme` default — no enforcement gate
+
+**File:** `docker-compose.yml:271`
+**Issue:** `GF_SECURITY_ADMIN_PASSWORD` falls back to the literal string `changeme`:
+```yaml
+- GF_SECURITY_ADMIN_PASSWORD=${GRAFANA_ADMIN_PASSWORD:-changeme}
+```
+If an operator deploys without setting `GRAFANA_ADMIN_PASSWORD` in `.env`, Grafana starts with a
+well-known default credential. Grafana has full access to Loki logs, which contain correlation IDs,
+path information, and potentially PII in error messages. There is no startup check that rejects
+the default. The CLAUDE.md Security Protocol requires "no default credentials".
+
+**Fix:** Remove the default fallback — use a strict variable reference so Docker Compose fails at
+startup if the variable is unset:
+```yaml
+- GF_SECURITY_ADMIN_PASSWORD=${GRAFANA_ADMIN_PASSWORD:?GRAFANA_ADMIN_PASSWORD must be set in .env}
+```
+Apply the same pattern to `GF_SECURITY_ADMIN_USER`. Update `.env.example` accordingly.
+
+---
+
+### CR-03: `test_docuvault_app_cannot_create_table` silently skips when the role is absent — gate is toothless
+
+**File:** `backend/tests/test_migration_0006.py:269-288`
+**Issue:** The test that enforces "DML-only app role cannot run DDL" calls `pytest.skip()` when the
+`docuvault_app` role does not exist in the integration database:
+```python
+if row is None:
+    pytest.skip("docuvault_app role does not exist in the integration database")
+```
+A `SKIP` exit is indistinguishable from a passing test in the `pytest -q` summary the security gate
+uses (12 skipped — correct behavior). If an integration environment is misconfigured and the role
+was never created, the privilege-escalation gate silently passes instead of catching the gap. The
+skip also means the test cannot be run in CI without the exact role present — which makes the gate
+depend on an undocumented pre-condition rather than failing loudly.
+
+**Fix:** Change `pytest.skip` to `pytest.fail` so a missing role is treated as a configuration
+error that must be resolved before the gate passes. If the role genuinely cannot exist in all
+environments, add an explicit environment variable (`ASSERT_APP_ROLE_EXISTS=1`) that controls
+whether the absent-role branch is a skip or a failure, defaulting to failure:
+```python
+if row is None:
+    msg = (
+        "docuvault_app role does not exist in the integration database. "
+        "Create the role with USAGE-only privilege or set SKIP_ROLE_CHECK=1 to bypass."
+    )
+    if os.environ.get("SKIP_ROLE_CHECK") == "1":
+        pytest.skip(msg)
+    pytest.fail(msg)
+```
+
+---
+
+## Warnings
+
+### WR-01: `migrate` source-code volume defeats `read_only: true`
+
+**File:** `docker-compose.yml:107-108`
+**Issue:** The migrate service mounts the entire backend source tree:
+```yaml
+volumes:
+  - ./backend:/app
+```
+This bind-mount is writable by default. A compromised or buggy migration script running under this
+service could modify source files on the host (`/app/alembic/`, `main.py`, etc.) even though the
+container filesystem is otherwise read-only. The `read_only: true` flag only protects the container
+layer, not the bind-mounted host path. The same issue exists for `backend`, `celery-worker`, and
+`celery-beat`, but is most severe for `migrate` because Alembic runs as the DDL-capable role.
+
+**Fix:** For the `migrate` service, mount the backend directory read-only:
+```yaml
+volumes:
+  - ./backend:/app:ro
+  - /tmp   # if alembic needs to write temp files, use a tmpfs
+```
+This is already present for the postgres initdb volume at line 10 (`:ro`). Apply the same pattern
+to the migrate service. Production deployments should bake the migration scripts into the image and
+omit the bind-mount entirely.
+
+---
+
+### WR-02: Three services use `latest` image tags — non-deterministic deployments
+
+**File:** `docker-compose.yml:19, 245, 254, 264`
+**Issue:** `minio/minio:latest`, `grafana/loki:latest`, `grafana/promtail:latest`, and
+`grafana/grafana:latest` all use floating `latest` tags. CLAUDE.md mandates pinned exact versions
+for security-critical packages. A `latest` tag can silently introduce breaking changes or
+vulnerabilities on the next `docker compose pull`. The SECURITY.md security gate checklist requires
+`docker scout` CVE scanning, which only produces reproducible results against pinned versions.
+
+**Fix:** Pin each image to a specific digest or semver tag:
+```yaml
+minio:
+  image: minio/minio:RELEASE.2024-11-07T00-52-20Z
+loki:
+  image: grafana/loki:3.3.2
+promtail:
+  image: grafana/promtail:3.3.2
+grafana:
+  image: grafana/grafana:11.4.0
+```
+
+---
+
+### WR-03: `minio-init` service has no container hardening
+
+**File:** `docker-compose.yml:40-87`
+**Issue:** The `minio-init` service runs an inline shell script that holds both the MinIO root
+credentials and the application access key/secret. Unlike every other service, it has no
+`read_only`, `cap_drop`, `security_opt`, or `restart` policy. If this container is compromised or
+the shell script is manipulated via a volume-mount attack, the attacker has full MinIO admin access
+(root credentials are present). It also has no `restart: "no"`, meaning Docker's default restart
+policy could cause it to re-run.
+
+**Fix:**
+```yaml
+minio-init:
+  image: minio/mc:latest
+  restart: "no"
+  cap_drop:
+    - ALL
+  security_opt:
+    - "no-new-privileges:true"
+  read_only: true
+  tmpfs:
+    - "/tmp:mode=1777"
+```
+
+---
+
+### WR-04: Integration test class relies on implicit execution order without ordering enforcement
+
+**File:** `backend/tests/test_migration_0006.py:137-288`
+**Issue:** `TestMigration0005To0006` contains 12 test methods that must run in definition order
+(`test_upgrade_to_0005` before `test_0005_lacks_phase12_tables` before `test_upgrade_to_head` before
+schema-assertion tests). pytest does not guarantee execution order within a class by default,
+especially when collected by multiple workers (`pytest-xdist`) or when tests are re-ordered by
+plugins. If `test_upgrade_to_head` runs before `test_upgrade_to_0005`, the "lacks_phase12_tables"
+assertions will fail spuriously — or worse, pass because the DB was already at head from a prior
+test run, hiding a real regression.
+
+The module-scoped `pg_conn` fixture also means any test that leaves the DB in a dirty state
+(partial upgrade) will corrupt all subsequent tests in the module.
+
+**Fix:** Add `@pytest.mark.order` (via `pytest-ordering` plugin) or restructure as sequential
+functions with explicit `depends` markers. Alternatively, use a single test function that sequences
+the steps explicitly and rolls back via a fixture teardown:
+```python
+def test_full_0005_to_0006_upgrade_cycle(migrate_dsn, pg_conn):
+    _run_alembic("0005", dsn=migrate_dsn)
+    # assert 0005 state
+    _run_alembic("head", dsn=migrate_dsn)
+    # assert 0006 state
+```
+
+---
+
+### WR-05: `celery-beat` missing `CLOUD_CREDS_KEY`, `JWT_PRIVATE_KEY`, `JWT_PUBLIC_KEY` env vars
+
+**File:** `docker-compose.yml:212-243`
+**Issue:** The `celery-beat` service environment block (lines 212–223) is missing
+`CLOUD_CREDS_KEY`, `JWT_PRIVATE_KEY`, and `JWT_PUBLIC_KEY`. These are present in `celery-worker`
+(lines 171–186). If a scheduled Celery beat task ever needs to decrypt cloud credentials (e.g., a
+future refresh task scheduled via beat rather than triggered from the browse endpoint) or verify a
+JWT, it will raise a `KeyError` or produce an unintelligible `settings` validation error at runtime
+rather than at startup. `celery-worker` already carries all three, indicating the intent is for all
+Celery processes to have access to them.
+
+**Fix:** Add the missing variables to `celery-beat`:
+```yaml
+celery-beat:
+  environment:
+    - CLOUD_CREDS_KEY=${CLOUD_CREDS_KEY}
+    - JWT_PRIVATE_KEY=${JWT_PRIVATE_KEY}
+    - JWT_PUBLIC_KEY=${JWT_PUBLIC_KEY}
+```
+
+---
+
+## Info
+
+### IN-01: README version (0.2.0) diverges from backend/main.py and AGENTS.md (0.2.1)
+
+**File:** `README.md:3`
+**Issue:** README.md reports `Version 0.2.0 — Alpha` while `backend/main.py:247` and
+`frontend/package.json` (version `0.2.1`) and `AGENTS.md` all reflect `0.2.1`. CLAUDE.md mandates
+a version bump in both `backend/main.py` and `frontend/package.json` and notes these are the
+"sources of truth". README was not updated after the Phase 12 gap-closure patch bump.
+
+**Fix:** Update `README.md:3` to `**Version 0.2.1 — Alpha**`.
+
+---
+
+### IN-02: `backend` service unnecessarily receives `DATABASE_MIGRATE_URL`
+
+**File:** `docker-compose.yml:129`
+**Issue:** The `backend` service environment block includes `DATABASE_MIGRATE_URL`. The application
+service uses `DATABASE_URL` (DML-only role). Providing the DDL-capable migrate URL to the
+application process means that if any code path in the app accidentally reads `DATABASE_MIGRATE_URL`
+from the environment and uses it (e.g., a misconfigured Alembic `env.py` fallback), it would
+operate with DDL privileges. The principle of least privilege requires the application service to
+hold only the credentials it needs.
+
+**Fix:** Remove `DATABASE_MIGRATE_URL` from the `backend` (and `celery-worker`) environment blocks.
+It is only needed by the `migrate` service.
+
+---
+
+### IN-03: `/health` endpoint leaks Python exception class names in response body
+
+**File:** `backend/main.py:299, 308`
+**Issue:** The health endpoint renders exceptions as `f"error: {type(e).__name__}: {e}"`:
+```python
+checks["postgres"] = f"error: {type(e).__name__}: {e}"
+checks["minio"] = f"error: {type(e).__name__}: {e}"
+```
+The inline comment at line 289 acknowledges this ("acceptable for an internal/dev endpoint in
+Phase 1. Phase 2 will trim…"), but Phase 12 is complete and the endpoint is still exposing
+exception internals. Exception messages from psycopg or the MinIO SDK may include connection
+strings, hostnames, or partial credentials. The CLAUDE.md Security Protocol requires "no stack
+traces in API responses".
+
+**Fix:** Trim the error payload to a safe, opaque string for any path that could be publicly
+reachable:
+```python
+checks["postgres"] = "error"
+checks["minio"] = "error"
+```
+Log the full exception to structlog at WARNING level for observability without exposing it in the
+response body.
+
+---
+
+_Reviewed: 2026-06-20_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_